- Revision
- 248909
- Author
- wenson_hs...@apple.com
- Date
- 2019-08-20 11:46:36 -0700 (Tue, 20 Aug 2019)
Log Message
Clicking the search icon on ae.com hangs the web content process
https://bugs.webkit.org/show_bug.cgi?id=200889
<rdar://problem/54359330>
Reviewed by Ryosuke Niwa.
Source/WebCore:
The hang occurs under FrameSelection::selectionAtSentenceStart, while computing an EditorState to send to the UI
process. This act of determining whether the given positon is at the start of sentence entails moving backwards
from the start of the current visible selection until the start of a paragraph or sentence is found, using
VisiblePosition::previous to iterate backwards through VisiblePositions.
However, on this website, VisiblePosition::previous ends up just returning the current position, and we loop
infinitely as a result because we never actually move backwards. This happens because VisiblePosition::previous
first uses previousVisuallyDistinctCandidate to find a candidate Position before the current position, but when
the position is canonicalized to create a VisiblePosition, it is moved back to its original Position as the deep
equivalent.
In the attached test case (which is representative of the relevant part of the DOM on ae.com), we try to find
the previous VisiblePosition from (#c, 0). The previous visually distinct candidate we initially find is
(#b, 0), since:
1. The enclosing renderer is a RenderBlock with a non-zero height.
2. The enclosing renderer has no rendered children.
3. The position is at the first editing position in the node (i.e. the span element).
However, when canonicalizing the position, we find that neither the upstream nor the downstream position is a
candidate because both the upstream and downstream nodes end up well outside of the span (the upstream node ends
up being at the start of the body element, and the downstream position ends up right before the start of #c's
container). The downstream position is at the end of a text node with a leading newline, it's not a candidate
because its last caret offset is less than the length of the text node.
As a result, even though the given position (#b, 0) is a candidate itself, its downstream and upstream positions
are not. Thus, VisiblePosition::canonicalPosition expands the scope of its candidate positions to the next
closest candidate positions; the next candidate position is (#c, 0). Both of these candidates are outside of the
containing block, so we (somewhat arbitrarily) break the tie by choosing the next visible position, bringing us
back to (#c, 0).
There are several ways to fix this, one of which involves fixing the downstream/upstream positions of (#b, 0) so
that they no longer jump out of the containing block of #b and cause (#b, 0) to be an invalid visible position
despite being a candidate position. This can be achieved by adjusting the heuristic in
endsOfNodeAreVisuallyDistinctPositions (used when moving upstream or downstream). Currently, this helper
function returns false for #b because they contain a single (non-rendered) whitespace character. Removing this
extraneous whitespace character actually causes the problem to stop reproducing, since #b and #c no longer
contain any child nodes. This is important because the heuristic in Position::downstream attempts to keep the
downstream position within the confines of the enclosing visual boundary, which (currently) ends up being the
entire body element because endsOfNodeAreVisuallyDistinctPositions returns false for #b.
To avoid this scenario, we teach endsOfNodeAreVisuallyDistinctPositions to treat inline-block containers that
are empty (that is, contain no rendered content) but may have children for editing in the same way as inline-
block containers that don't have any children; in both scenarios, they may contain a candidate position, so we
should treat the ends of the container node as being visually distinct.
Doing so causes the downstream position of (#b, 0) to be kept within the immediate containing span element,
which then allows (#b, 0) to be a canonical VisiblePosition.
Tests: fast/events/focus-anchor-with-tabindex-hang.html
editing/selection/modify-backward-inline-block-containers.html
* editing/VisiblePosition.cpp:
(WebCore::VisiblePosition::previous const):
LayoutTests:
* editing/selection/modify-backward-inline-block-containers-expected.txt: Added.
* editing/selection/modify-backward-inline-block-containers.html: Added.
Add a layout test to ensure that the selection may be moved through empty inline-block containers that span the
width of the page.
* fast/events/focus-anchor-with-tabindex-hang-expected.txt: Added.
* fast/events/focus-anchor-with-tabindex-hang.html: Added.
Add a layout test to ensure that clicking an empty span under a focusable anchor element moves focus to the
anchor element instead of hanging the web content process or hitting a debug assertion.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (248908 => 248909)
--- trunk/LayoutTests/ChangeLog 2019-08-20 17:54:02 UTC (rev 248908)
+++ trunk/LayoutTests/ChangeLog 2019-08-20 18:46:36 UTC (rev 248909)
@@ -1,3 +1,23 @@
+2019-08-20 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Clicking the search icon on ae.com hangs the web content process
+ https://bugs.webkit.org/show_bug.cgi?id=200889
+ <rdar://problem/54359330>
+
+ Reviewed by Ryosuke Niwa.
+
+ * editing/selection/modify-backward-inline-block-containers-expected.txt: Added.
+ * editing/selection/modify-backward-inline-block-containers.html: Added.
+
+ Add a layout test to ensure that the selection may be moved through empty inline-block containers that span the
+ width of the page.
+
+ * fast/events/focus-anchor-with-tabindex-hang-expected.txt: Added.
+ * fast/events/focus-anchor-with-tabindex-hang.html: Added.
+
+ Add a layout test to ensure that clicking an empty span under a focusable anchor element moves focus to the
+ anchor element instead of hanging the web content process or hitting a debug assertion.
+
2019-08-20 Ryan Haddad <ryanhad...@apple.com>
Web Inspector: Support for _javascript_ BigInt
Added: trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers-expected.txt (0 => 248909)
--- trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers-expected.txt 2019-08-20 18:46:36 UTC (rev 248909)
@@ -0,0 +1,156 @@
+This test verifies that the selection can be moved forwards and backwards through empty inline-block containers that span the full width of the document. To manually test, use arrow keys or click before each exclamation mark to verify that the selection can be moved into each inline-block span element.
+
+Initial caret position:
+| "
+ "
+| <a>
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| "
+ "
+| <div>
+| "
+ "
+| <span>
+| class="empty"
+| id="start"
+| <#selection-caret>
+| " "
+| "
+ "
+| "
+ "
+
+Move backward:
+| "
+ "
+| <a>
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| <span>
+| class="empty"
+| <#selection-caret>
+| " "
+| "
+ "
+| "
+ "
+| <div>
+| "
+ "
+| <span>
+| class="empty"
+| id="start"
+| " "
+| "
+ "
+| "
+ "
+
+Move backward again:
+| "
+ "
+| <a>
+| "
+ "
+| <span>
+| class="empty"
+| <#selection-caret>
+| " "
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| "
+ "
+| <div>
+| "
+ "
+| <span>
+| class="empty"
+| id="start"
+| " "
+| "
+ "
+| "
+ "
+
+Move forward:
+| "
+ "
+| <a>
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| <span>
+| class="empty"
+| <#selection-caret>
+| " "
+| "
+ "
+| "
+ "
+| <div>
+| "
+ "
+| <span>
+| class="empty"
+| id="start"
+| " "
+| "
+ "
+| "
+ "
+
+Move forward again:
+| "
+ "
+| <a>
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| <span>
+| class="empty"
+| " "
+| "
+ "
+| "
+ "
+| <div>
+| "
+ "
+| <span>
+| class="empty"
+| id="start"
+| <#selection-caret>
+| " "
+| "
+ "
+| "
+ "
Added: trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers.html (0 => 248909)
--- trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers.html (rev 0)
+++ trunk/LayoutTests/editing/selection/modify-backward-inline-block-containers.html 2019-08-20 18:46:36 UTC (rev 248909)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+ <head>
+ <style>
+ span.empty {
+ display: inline-block;
+ width: 100%;
+ font-size: 40px;
+ }
+
+ span.empty:before {
+ content: "!";
+ }
+ </style>
+ <script src=""
+ <script>
+ addEventListener("DOMContentLoaded", () => {
+ document.designMode = "on";
+ Markup.description("This test verifies that the selection can be moved forwards and backwards through empty inline-block containers that span the full width of the document. To manually test, use arrow keys or click before each exclamation mark to verify that the selection can be moved into each inline-block span element.");
+
+ getSelection().setPosition(document.getElementById("start"));
+ Markup.dump("container", "Initial caret position");
+
+ getSelection().modify("move", "backward", "character");
+ Markup.dump("container", "Move backward");
+
+ getSelection().modify("move", "backward", "character");
+ Markup.dump("container", "Move backward again");
+
+ getSelection().modify("move", "forward", "character");
+ Markup.dump("container", "Move forward");
+
+ getSelection().modify("move", "forward", "character");
+ Markup.dump("container", "Move forward again");
+ });
+ </script>
+ </head>
+ <body>
+ <p id="description"></p>
+ <div id="container">
+ <a>
+ <span class="empty"> </span>
+ <span class="empty"> </span>
+ </a>
+ <div>
+ <span class="empty" id="start"> </span>
+ </div>
+ </div>
+ </body>
+</html>
Added: trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang-expected.txt (0 => 248909)
--- trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang-expected.txt 2019-08-20 18:46:36 UTC (rev 248909)
@@ -0,0 +1,12 @@
+This test verifies that clicking an empty container in an anchor element with a tabindex correctly focuses the anchor without hanging the web content process. To run the test manually, tap or click the red box; the active element should be set to the anchor element, and the web process should remain responsive.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Clicked target.
+PASS document.activeElement is anchor
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
Added: trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang.html (0 => 248909)
--- trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang.html (rev 0)
+++ trunk/LayoutTests/fast/events/focus-anchor-with-tabindex-hang.html 2019-08-20 18:46:36 UTC (rev 248909)
@@ -0,0 +1,56 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+ <head>
+ <meta name="viewport" content="width=device-width, initial-scale=1">
+ <style>
+ a[tabindex] {
+ outline: none;
+ }
+
+ span.empty {
+ display: inline-block;
+ width: 100%;
+ height: 1em;
+ font-size: 40px;
+ }
+
+ span#b {
+ border: 1px solid red;
+ }
+ </style>
+ <script src=""
+ <script src=""
+ <script>
+ jsTestIsAsync = true;
+
+ addEventListener("load", async () => {
+ anchor = document.querySelector("a");
+ target = document.getElementById("b");
+ target.addEventListener("click", () => {
+ testPassed("Clicked target.");
+ shouldBe("document.activeElement", "anchor");
+ });
+
+ description("This test verifies that clicking an empty container in an anchor element with a tabindex correctly focuses the anchor without hanging the web content process. To run the test manually, tap or click the red box; the active element should be set to the anchor element, and the web process should remain responsive.");
+
+ if (!window.testRunner)
+ return;
+
+ await UIHelper.activateElement(target);
+ await UIHelper.ensurePresentationUpdate();
+ finishJSTest();
+ });
+ </script>
+ </head>
+ <body>
+ <p id="description"></p>
+ <p id="console"></p>
+ <a tabindex="0">
+ <span class="empty" id="a"> </span>
+ <span class="empty" id="b"> </span>
+ </a>
+ <div>
+ <span class="empty" id="c"> </span>
+ </div>
+ </body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (248908 => 248909)
--- trunk/Source/WebCore/ChangeLog 2019-08-20 17:54:02 UTC (rev 248908)
+++ trunk/Source/WebCore/ChangeLog 2019-08-20 18:46:36 UTC (rev 248909)
@@ -1,3 +1,66 @@
+2019-08-20 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Clicking the search icon on ae.com hangs the web content process
+ https://bugs.webkit.org/show_bug.cgi?id=200889
+ <rdar://problem/54359330>
+
+ Reviewed by Ryosuke Niwa.
+
+ The hang occurs under FrameSelection::selectionAtSentenceStart, while computing an EditorState to send to the UI
+ process. This act of determining whether the given positon is at the start of sentence entails moving backwards
+ from the start of the current visible selection until the start of a paragraph or sentence is found, using
+ VisiblePosition::previous to iterate backwards through VisiblePositions.
+
+ However, on this website, VisiblePosition::previous ends up just returning the current position, and we loop
+ infinitely as a result because we never actually move backwards. This happens because VisiblePosition::previous
+ first uses previousVisuallyDistinctCandidate to find a candidate Position before the current position, but when
+ the position is canonicalized to create a VisiblePosition, it is moved back to its original Position as the deep
+ equivalent.
+
+ In the attached test case (which is representative of the relevant part of the DOM on ae.com), we try to find
+ the previous VisiblePosition from (#c, 0). The previous visually distinct candidate we initially find is
+ (#b, 0), since:
+
+ 1. The enclosing renderer is a RenderBlock with a non-zero height.
+ 2. The enclosing renderer has no rendered children.
+ 3. The position is at the first editing position in the node (i.e. the span element).
+
+ However, when canonicalizing the position, we find that neither the upstream nor the downstream position is a
+ candidate because both the upstream and downstream nodes end up well outside of the span (the upstream node ends
+ up being at the start of the body element, and the downstream position ends up right before the start of #c's
+ container). The downstream position is at the end of a text node with a leading newline, it's not a candidate
+ because its last caret offset is less than the length of the text node.
+
+ As a result, even though the given position (#b, 0) is a candidate itself, its downstream and upstream positions
+ are not. Thus, VisiblePosition::canonicalPosition expands the scope of its candidate positions to the next
+ closest candidate positions; the next candidate position is (#c, 0). Both of these candidates are outside of the
+ containing block, so we (somewhat arbitrarily) break the tie by choosing the next visible position, bringing us
+ back to (#c, 0).
+
+ There are several ways to fix this, one of which involves fixing the downstream/upstream positions of (#b, 0) so
+ that they no longer jump out of the containing block of #b and cause (#b, 0) to be an invalid visible position
+ despite being a candidate position. This can be achieved by adjusting the heuristic in
+ endsOfNodeAreVisuallyDistinctPositions (used when moving upstream or downstream). Currently, this helper
+ function returns false for #b because they contain a single (non-rendered) whitespace character. Removing this
+ extraneous whitespace character actually causes the problem to stop reproducing, since #b and #c no longer
+ contain any child nodes. This is important because the heuristic in Position::downstream attempts to keep the
+ downstream position within the confines of the enclosing visual boundary, which (currently) ends up being the
+ entire body element because endsOfNodeAreVisuallyDistinctPositions returns false for #b.
+
+ To avoid this scenario, we teach endsOfNodeAreVisuallyDistinctPositions to treat inline-block containers that
+ are empty (that is, contain no rendered content) but may have children for editing in the same way as inline-
+ block containers that don't have any children; in both scenarios, they may contain a candidate position, so we
+ should treat the ends of the container node as being visually distinct.
+
+ Doing so causes the downstream position of (#b, 0) to be kept within the immediate containing span element,
+ which then allows (#b, 0) to be a canonical VisiblePosition.
+
+ Tests: fast/events/focus-anchor-with-tabindex-hang.html
+ editing/selection/modify-backward-inline-block-containers.html
+
+ * editing/VisiblePosition.cpp:
+ (WebCore::VisiblePosition::previous const):
+
2019-08-20 Brent Fulgham <bfulg...@apple.com>
[FTW] Fix scrolling in modern WebKit views
Modified: trunk/Source/WebCore/dom/Position.cpp (248908 => 248909)
--- trunk/Source/WebCore/dom/Position.cpp 2019-08-20 17:54:02 UTC (rev 248908)
+++ trunk/Source/WebCore/dom/Position.cpp 2019-08-20 18:46:36 UTC (rev 248909)
@@ -625,8 +625,14 @@
if (is<HTMLTableElement>(*node))
return false;
+ if (!node->renderer()->isReplaced() || !canHaveChildrenForEditing(*node) || !downcast<RenderBox>(*node->renderer()).height())
+ return false;
+
// There is a VisiblePosition inside an empty inline-block container.
- return node->renderer()->isReplaced() && canHaveChildrenForEditing(*node) && downcast<RenderBox>(*node->renderer()).height() && !node->firstChild();
+ if (!node->hasChildNodes())
+ return true;
+
+ return !Position::hasRenderedNonAnonymousDescendantsWithHeight(downcast<RenderElement>(*node->renderer()));
}
static Node* enclosingVisualBoundary(Node* node)
