Snav: Fix faulty focus jump when exiting scrollable area [chromium/src : master]
Hugo Holgersson (Gerrit)
Hi JunHo! I think I got a fix for the bug you filed. What do you think?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
JunHo Seo (Gerrit)
Good work! Thanks!
1 comment:
File third_party/WebKit/Source/core/page/FocusController.cpp:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();How about add !HasOffscreenRect(container).
And I think it will be better if we use container's virtual & visual rect with intersection of focused element.
For example, if we hit down key, starting rect will be
(max(focused_element.left, virtual&visual(container).left), // => A
virtual&visual(container).top,
min(focused_element.(left + width), virtual&visual(container).(left + width)) - A,
0)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();
I don't understand what you mean with:
virtual & visual
Could you draw a picture of the overlapping rectangles? :)
How about add !HasOffscreenRect(container).
How about this:
if focused element is not visible at all AND the container is (partially) visible:
use container's visible rect as starting_rect
else:
use the frame's visual viewport as starting_rect?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
JunHo Seo (Gerrit)
Hugo, would please see my reply?
1 comment:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();
Could you draw a picture of the overlapping rectangles? :)
Please refer to:
https://docs.google.com/document/d/e/2PACX-1vSrEh7UVEabgv8ra7VEXp1_eb9x8gVwpYNe52_8jaKsSBB2Anu7MgejqwSTfkAyQGsTvz8hz3cse_28/pubHow about this:
Do you mean:
if focused element is visible:
use focused element's visible rect
else if focused element is not visible AND the container is visible:
use container's visible rect [A]
else // focused element is not visible AND the container is not visible:
use frame's visual viewport
?I agree. Just I want to use that I suggested in condition [A] :)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();
I thought:
container->GetLayoutObject()->AbsoluteVisualRect() == container's visible rect
I see in the picture that you've projected the focused element onto the container's edge line?
That does make sense. But I wonder if we need such accuracy? :P We would then need logic to do projections in all 4 directions? Maybe this is enough:
if focused element is visible:
use focused element's AbsoluteVisualRect
else if focused element is not visible AND the container is visible:
use container's AbsoluteVisualRect
else // focused element is not visible AND the container is not visible:
use FindSearchStartPoint(frame)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
JunHo Seo (Gerrit)
1 comment:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();
But I wonder if we need such accuracy?
I'll have more investigation about that. Please give the work to me :)
At least, I think we should use VirtualRectForDirection for second case.
if focused element is visible:
use focused element's AbsoluteVisualRect
else if focused element is not visible AND the container is visible:
use VirtualRectForDirection(container's AbsoluteVisualRect)
else // focused element is not visible AND the container is not visible:
use FindSearchStartPoint(frame)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
if (focused_element && HasOffscreenRect(focused_element))
starting_rect = container->GetLayoutObject()->AbsoluteVisualRect();
I'll have more investigation about that. Please give the work to me :)
Ok! I'll drop this CL and assign you to this bug. I'm very busy these days anyway ;)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #3 to this change.
Snav: Fix faulty focus jump when exiting scrollable area
Conditions:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Now we make spatnav start searching from the edge of A
that faces the navigated direction. The edge must be
visible in the visual viewport.
If both F and its enclosing container A is completely
offscreen, we recurse to the container’s container up until
the document root. The document root node is a good base
case because it is a container that’s always visible.
Previously, we didn't handle the "focused elemement is
offscreen"-case at all.
For more details, see http://bit.ly/snav2.
Bug: 804669
---
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div-expected.txt
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
M third_party/WebKit/Source/core/page/FocusController.cpp
M third_party/WebKit/Source/core/page/SpatialNavigation.cpp
M third_party/WebKit/Source/core/page/SpatialNavigation.h
5 files changed, 135 insertions(+), 54 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
if (container && container->IsDocumentNode())
ToDocument(container)->UpdateStyleAndLayoutIgnorePendingStylesheets> I'll have more investigation about that. Please give the work to me :) […]
I got a new idea for this CL! :)
PTAL at PS3. I hope it doesn't conflict with what you had in mind? If it does, maybe this solution is good enough to begin with? At least it solves the bug's two test cases...
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #11 to this change.
Snav: Fix faulty focus jump when exiting scrollable area
Conditions for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge,
not from A's edge.
Solution:
Set the "search origin" (the location in the root
document's scrollport where spatnav should start its
search) to one of A's outer edges.
Example:
UP makes spatnav search upwards from A's bottommost edge.
If both F and its enclosing scroller A are completely
offscreen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition,a visible scrollable area.
Testing:
This CL introduces assertMovements() that allows us to
write testharness.js-tests with the same old, compact and
declarative "step focus and check result"-syntax our
existing tests use. assertMovements() could also help us
port those old snav* layout tests into using testharness.js.
Bug: 770147, 803086, 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
M third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/spatial-navigation-utils.js
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-use-visual-viewport-upwards.html
M third_party/WebKit/Source/core/page/FocusController.cpp
M third_party/WebKit/Source/core/page/SpatialNavigation.cpp
M third_party/WebKit/Source/core/page/SpatialNavigation.h
M third_party/WebKit/Source/core/page/SpatialNavigationTest.cpp
7 files changed, 273 insertions(+), 61 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson would like Stefan Zager to review this change.
7 files changed, 272 insertions(+), 61 deletions(-)
JunHo Seo (Gerrit)
Hi hugo.
I'm sorry too late reply.
And thank you very much for this fix. I like this CL very much :)
Would you please see my inline comments?
10 comments:
File third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/spatial-navigation-utils.js:
function assertMovements(resultMap)
{nit: what is proper coding style for '{' ?
You added '{' differently for two function.Although most functions in this file have line feed before '{', I think removing line feed is more right coding style. How do you think?
Patch Set #13, Line 124:
switch (direction) {Existing test cases added ["DONE", "DONE"] element at last of resultMap.
Do you have a plan remove this line?
File third_party/WebKit/Source/core/page/SpatialNavigation.h:
Patch Set #13, Line 151:
LayoutRect SearchOrigin(Document*,Maybe CORE_EXPORT is needed for functions that are used in unit test?
Patch Set #13, Line 154:
bool start_at_containerHow about add default parameter for start_at_container(+ !variable name is different with cpp)?
And then we use just default parameter at FocusController and Unit test.
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp:
Patch Set #9, Line 678:
case kWebFocusTypeUp:nit: How about process Up case first then process Down case later?
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp:
// A rectangle, as close to |start_box| as possible, in the root frame's
// coordinate space where spatnav should start its search given
// that |start_box| is focused. Spatnav uses this rectangle to meassure
// distances to "focus candidates".I hope to give an information about 'We try to find a first (partially) onscreen element from a element chain(focused element ~ document)'.
Patch Set #13, Line 720:
bool box_is_containerThis argument may has incorrect value when current focused element is container.
Would you please think about more proper variable name?
Hmm... how about should_take_opposite_side_of_box? :)// When no element has focus, search from one of the root frame's view's edges
// towards the navigated direction. For example, UP makes spatnav search
// upwards, starting at the root frame's view's bottom.
// TODO(crbug.com/823227):
// When pinch-zoomed, search from one of the visual viewport's edges instead.
if (!start_box || start_box->IsDocumentNode())
return OppositeSideOfBox(focused_document, direction);Good handling edge case, good TODO.
File third_party/WebKit/Source/core/page/SpatialNavigationTest.cpp:
I think this is typo :) searchs_start -> search_start
ditto + line 120, 132, etc
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Thanks!
7 comments:
function assertMovements(resultMap)
{
nit: what is proper coding style for '{' ? […]
We should probably follow WPT's style... I might move assertMovements() out to a file of its own and maybe put it in third_party/WebKit/LayoutTests/external/wpt_automation (if it's accepted there).
Patch Set #13, Line 124:
switch (direction) {
Existing test cases added ["DONE", "DONE"] element at last of resultMap. […]
Yes, that DONE,DONE-line is not needed anymore.
File third_party/WebKit/Source/core/page/SpatialNavigation.h:
Patch Set #13, Line 151:
LayoutRect SearchOrigin(Document*,Maybe CORE_EXPORT is needed for functions that are used in unit test?
I'm not sure. At least webkit_unit_tests compiles locally on my machine. I will give it a try on the bots.
Patch Set #13, Line 154:
bool start_at_container
How about add default parameter for start_at_container(+ !variable name is different with cpp)? […]
At first I actually used a default value, but after reading https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/XebqRcjAcco I was unsure...
But yep, it's true that clients must always call SearchOrigin() with 'false'. 'true' is only set by recursive calls.
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp:
// A rectangle, as close to |start_box| as possible, in the root frame's
// coordinate space where spatnav should start its search given
// that |start_box| is focused. Spatnav uses this rectangle to meassure
// distances to "focus candidates".
I hope to give an information about 'We try to find a first (partially) onscreen element from a elem […]
Ok... I will try to add some more info to this comment.
Patch Set #13, Line 720:
bool box_is_container
This argument may has incorrect value when current focused element is container. […]
Good idea.
File third_party/WebKit/Source/core/page/SpatialNavigationTest.cpp:
I think this is typo :) searchs_start -> search_start
I actually meant search's_start (but the ' isn't allowed in variable names).
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #15 to this change.
Snav: Fix faulty focus jump when exiting scrollable area
Conditions for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge,
not from A's edge.
Solution:
Set the "search origin" (the location in the root
document's scrollport where spatnav should start its
search) to one of A's outer edges.
Example:
UP makes spatnav search upwards from A's bottommost edge.
If both F and its enclosing scroller A are completely
offscreen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition,a visible scrollable area.
Bug: 770147, 803086, 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
M third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/snav-testharness.js
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-use-visual-viewport-upwards.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
7 files changed, 241 insertions(+), 64 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
4 comments:
We should probably follow WPT's style... […]
Done. (I landed spatial-navigation-utils.js separately).
Yes, that DONE,DONE-line is not needed anymore.
Done. (I landed spatial-navigation-utils.js separately).
File third_party/WebKit/Source/core/page/SpatialNavigation.cpp:
Ok... I will try to add some more info to this comment.
Done.
Good idea.
I realized we don't need to pass this argument at all... See PS16.
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #17 to this change.
Snav: Define SearchOrigin as "self or first visible container"
Conditions for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge,
not from A's edge.
Solution:
Set the "search origin" (the location in the root
document's scrollport where spatnav should start its
search) to one of A's outer edges.
Example:
UP makes spatnav search upwards from A's bottommost edge.
If both F and its enclosing scroller A are completely
off-screen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition,a visible scrollable area.
Bug: 770147, 803086, 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
M third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/snav-testharness.js
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-use-visual-viewport-upwards.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
7 files changed, 239 insertions(+), 64 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #18 to this change.
Snav: Define SearchOrigin as "self or first visible container"
Conditions for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge,
not from A's edge.
Solution:
Set the "search origin" (the location in the root document's
scrollport where spatnav should start its search) to one of
A's outer edges.
Example:
UP makes spatnav search upwards from A's bottommost edge.
If both F and its enclosing scroller A are completely
off-screen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition, a visible scrollable area.
Bug: 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
M third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/snav-testharness.js
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-use-visual-viewport-upwards.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
7 files changed, 239 insertions(+), 64 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
JunHo Seo (Gerrit)
Hi, hugo. Would you please take a look my comment?
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Can't we do this using start_box? Then we can remove one argument(focused_document). Moreover, it looks problematic if SearchOrigin starts inside iframe:
1) focused_document -> points iframe's document
2) line 739: container -> points main document
3) Recursive call SearchOrigin: start_box points main document.In above case, we should use main document's OppositeSideOfBox().
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #20 to this change.
Snav: Define search origin as the first visible focus-container
This fixes two bugs:
804669: activeElement is ignored if being off-screen
823227: Focus goes to elements outside a pinch-zoomed viewport
Scenario for 804669:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge,
not from A's edge.
Solution:
Set the "search origin" to one of A's outer edges.
Example:
UP makes spatnav search upwards from A's bottommost edge.
To solve these problems we need to define the "search origin"
of spatial navigation. The search origin is either
activeElement itself, if it's being at least partially visible,
or its first [partially] visible scroller.
If both F and its enclosing scroller A are completely
off-screen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition, a visible scrollable area.
Bug: 804669, 823227
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
M third_party/WebKit/LayoutTests/fast/spatial-navigation/resources/snav-testharness.js
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-exit-overflow-div.html
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-use-visual-viewport-upwards.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
7 files changed, 386 insertions(+), 87 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
I've fixed the TODO of pinched visual viewports!
PTAL :)
2 comments:
Can't we do this using start_box? Then we can remove one argument(focused_document). […]
I've added a unit test, "PartiallyVisibleFrame", that covers this case.
I looked into fixing crbug.com/823227 as well. As I did that, I rewrote this CL quite a bit... Now we don't need OppositeSideOfBox() anymore. PTAL :)
I still pass focused_document, however. This is because RootViewport() *NEW* needs a hook into the DOM tree. RootViewport() returns the visual viewport's rect in the root frame's coordinate space. (There can only be one root frame...)
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #19, Line 707:
// The rect of the visual viewport given in the root frame's coordinate space.szager@, I have a feeling I re-invented something here. Perhaps there already exists a helper-function like this one?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
JunHo Seo (Gerrit)
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #19, Line 751:
IsScrollableAreaOrDocument(focus_box)Side question:
I'm feeling confusing about when we should use IsNavigableContainer() or IsScrollableAreaOrDocument()?
I think we should use only one check, and IsScrollableAreaOrDocument() is more suitable. So I suggest replace all IsNavigableContainer() to IsScrollableAreaOrDocument(). What do you think?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #23 to this change.
Snav: Define search origin as the first visible focus-container
Scenario for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge, not
from A's edge.
Solution:
Set the "search origin" to one of A's outer edges.
To solve this problem we redefine the "search origin" of
spatial navigation. The search origin is either activeElement
itself, if it's being at least partially visible, or its
first [partially] visible scroller.
If both F and its enclosing scroller A are completely
offscreen, we recurse to the scroller’s scroller up until
the document root. The document root node is a good base
case because it's, per definition, a visible scrollable area.
This builds onto the idea of:
https://chromium-review.googlesource.com/c/chromium/src/+/873645
Bug: 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-search-beneath-exited-scroller.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
5 files changed, 389 insertions(+), 58 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
I fixed the visual viewport bug in its own CL; https://chromium-review.googlesource.com/c/chromium/src/+/1204094 instead.
Now this CL should be more review-friendly. Adding fs@ as Stefan seems busy.
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #19, Line 751:
ox_in_root_frame, RootViewport(focuseSide question: […]
That would change semantics. FindFocusCandidateInContainer calls IsNavigableContainer because it takes "scrollability in *direction*" into account.
When it comes to search origin, we're only interested in scrollers' visibility (not scrollability).
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
9 comments:
Patch Set #23, Line 1:
<script src="../../resources/testharness.js"></script>Add a doctype unless this is supposed to be a quirks mode specific test (which probably should be clearly indicated if so.)
File third_party/blink/renderer/core/page/focus_controller.cc:
const LayoutRect start_box =
SearchOrigin(focused_document, focused_element, direction)You moved this before the call to UpdateStyleAndLayoutIgnorePendingStylesheets, which would seem to imply that the SearchOrigin could be based on old layout information? Perhaps this should go after that line instead? (Layout can theoretically affect the focused element, but will however only clear it on a timer, so FocusedElement could also be "wrong" here, but that's another issue...)
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #23, Line 657:
OppositeSideNit: OppositeEdge? ("edge" is the more common term here, and also used in CSS - i.e border edge et.c.)
Patch Set #23, Line 683:
StartLine...and thus this should maybe be either "StartSide..." or "StartEdge..." to keep the nomenclature consistent. (Since <area>s are shapes, that can have lines one might think this refers to that. If one were to care about <area>s at all that is =))
(However I wonder if it wouldn't be better to just open-code this in the FocusCandidate constructor, and then aim to restructure the computation of the "box" such that <area> is handled in one place and not three... food for thought, but not needed in this CL.)
LayoutRect doc_rect =
LayoutRect(Nit: Could be LayoutRect doc_rect(...) instead.
we recurse to the
// scroller’s scroller up until the document rootAre there tests for nested scrollable containers (and the case where the document node is reached)?
I wonder if it wouldn't be better to keep the walk of "scrolling ancestors" separate from the search area based on a potential focus element though? For example, shouldn't the "skipped" scrollable containers be added to the skip-list? Or would we potentially want to visit them again?
Patch Set #23, Line 720:
focus_boxNit: "box" is kind of overloaded, maybe this could be focus_node instead?
Patch Set #23, Line 740:
area_element->ImageElement()Nit: focus_box
Node* container = ScrollableAreaOrDocumentOf(focus_box);
return SearchOrigin(focused_document, container, direction)When we're potentially moving up the frame hierarchy here, shouldn't that be reflected here?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
Node* container = ScrollableAreaOrDocumentOf(focus_box);
return SearchOrigin(focused_document, container, direction)When we're potentially moving up the frame hierarchy here, shouldn't that be reflected here?
Yes we do. You mean reflected in the comment?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
1 comment:
Node* container = ScrollableAreaOrDocumentOf(focus_box);
return SearchOrigin(focused_document, container, direction)
Yes we do. […]
I mean as in update |focused_document|.
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
9 comments:
Patch Set #23, Line 1:
<!DOCTYPE html>Add a doctype unless this is supposed to be a quirks mode specific test (which probably should be cl […]
Done
File third_party/blink/renderer/core/page/focus_controller.cc:
Node* container = focused_document;
if (container->IsDocumentNode())You moved this before the call to UpdateStyleAndLayoutIgnorePendingStylesheets, which would seem to […]
I'm not sure why UpdateStyleAndLayoutIgnorePendingStylesheets is needed here at all, to be honest... OK. Just keeping this as it is. Done.
File third_party/blink/renderer/core/page/spatial_navigation.cc:
Patch Set #23, Line 657:
OppositeEdgeNit: OppositeEdge? ("edge" is the more common term here, and also used in CSS - i.e border edge et. […]
Done
Patch Set #23, Line 683:
StartEdge...and thus this should maybe be either "StartSide..." or "StartEdge... […]
Done
LayoutRect doc_rect(frame_view->GetScrollableArea()->VisibleContentRect());
LayoutRect fram
Nit: Could be LayoutRect doc_rect(...) instead.
Done
nt root node is a
// good base case because it's, per definition, a
Are there tests for nested scrollable containers (and the case where the document node is reached)?
True. I didn't add a test for nested scrollable <div>s or nested scrollable <iframe>s. I could do that...
For example, shouldn't the "skipped" scrollable containers be added to the skip-list?
Right. That could be one optimization. I should tell you, my long-term idea for spatnav is something like http://bit.ly/snav2 (which aims to solve crbug.com/801162).
This CL defines SearchOrigin() so it could be used by "snav2". Snav2 doesn't need a skip-list because it digs into one container at the time, which is more like how sequential navigation works.
Patch Set #23, Line 720:
usType di
Nit: "box" is kind of overloaded, maybe this could be focus_node instead?
Done
Nit: focus_box
Done
return SearchOrigin(focused_document, container, direction);
}
I mean as in update |focused_document|.
I was unsure about this. focused_document is only passed along to be used as an argument to RootViewport().
I want RootViewport() to return
// The rect of the visual viewport given in the root frame's coordinate space.
My thinking was that, no matter focused_document's depth, the visual viewport's rect is constant.
I guess I rather need to change RootViewport() to always use the _root_ frame's LocalFrameView and not just the frame of the passed, possibly nested, document? I.e use a "root_doc_rect" instead of "doc_rect".
Perhaps I don't even need to do that...
I have a feeling RootViewport() could be simplified a great deal with something like VisualViewport::VisibleRectInDocument() or RootFrameViewport::VisibleContentRect() ... ?
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
2 comments:
nt root node is a
// good base case because it's, per definition, a
> Are there tests for nested scrollable containers (and the case where the document node is reached) […]
Ok, fair enough. (Tests would still be good of course.)
return SearchOrigin(focused_document, container, direction);
}
I was unsure about this. […]
I believe that now it is more like "the viewport of |focused_document| in the (local) root's coordinate space". I think I'd expect to use the document of whatever the current scrollable container/element is in.
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
1 comment:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
return SearchOrigin(visible_root_frame, container, direction);
}
I think I'd expect to use the document of whatever the current scrollable container/element is in.
As I understand it, spatnav always works in (=compares distances between rects in) the (local) root frame's coordinate space. That's why we're only interested in the "root document" (and not in any "current document").
I believe that now it is more like "the viewport of |focused_document| in the (local) root's coordinate space".
Yes... I fixed this by using LocalFrame::LocalFrameRoot() in RootViewport(). That is, instead of passing in a document.
To not have to recalculate the root frame's visible rect, I pass it as visible_root_frame to SearchOrigin(). That makes SearchOrigin easier to test too :)
Note: The viewport rect we really want is the one of the tree's topmost frame, local or remote. AdvanceFocusDirectionally() mentions this in a FIXME. Seqnav can navigate to remote frames but spatnav can't... I'll turn that FIXME into a TODO(new bug).
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Patch set 25:Code-Review +1
Hugo Holgersson (Gerrit)
Hugo Holgersson uploaded patch set #29 to this change.
Snav: Define search origin as the first visible focus-container
Scenario for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge, not
from A's edge.
Solution:
Set the "search origin" to one of A's outer edges.
To solve this problem we redefine the "search origin" of
spatial navigation. The search origin is either activeElement
itself, if it's being at least partially visible, or its
first [partially] visible scroller.
If both F and its enclosing scroller A are completely
offscreen, we recurse to the scroller’s scroller up until
the root frame's document. The root document is a good base
case because it's, per definition, a visible scrollable area.
This builds onto the idea of:
https://chromium-review.googlesource.com/c/chromium/src/+/873645
Bug: 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
---
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-search-beneath-exited-scroller.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
5 files changed, 479 insertions(+), 64 deletions(-)
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
Patch set 29:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/883533/29
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/883533/29
Bot data: {"action": "start", "triggered_at": "2018-09-07T11:05:20.0Z", "cq_cfg_revision": "a56c78564b225bcb69e30926e5949eccff771e38", "revision": "6ce6377ab2fab35c8c2f5ac8546591c5b6649d21"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
android-marshmallow-arm64-rel on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/80421)
Hugo Holgersson (Gerrit)
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/883533/29
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/883533/29
Bot data: {"action": "start", "triggered_at": "2018-09-07T11:55:43.0Z", "cq_cfg_revision": "a56c78564b225bcb69e30926e5949eccff771e38", "revision": "6ce6377ab2fab35c8c2f5ac8546591c5b6649d21"}
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
+ David on CC, to let you know about the TODO I planned for IsRectOffscreen().
Commit Bot (Gerrit)
Commit Bot merged this change.
Snav: Define search origin as the first visible focus-container
Scenario for bug:
1. The focused element F is inside a scrollable area A.
2. F is offscreen (=clipped) but A is (partly) visible.
Problem:
Spatnav started to search from the document's edge, not
from A's edge.
Solution:
Set the "search origin" to one of A's outer edges.
To solve this problem we redefine the "search origin" of
spatial navigation. The search origin is either activeElement
itself, if it's being at least partially visible, or its
first [partially] visible scroller.
If both F and its enclosing scroller A are completely
offscreen, we recurse to the scroller’s scroller up until
the root frame's document. The root document is a good base
case because it's, per definition, a visible scrollable area.
This builds onto the idea of:
https://chromium-review.googlesource.com/c/chromium/src/+/873645
Bug: 804669
Change-Id: I15b0f41426d7632fe9ec62d77a414d592e3631c0
Reviewed-on: https://chromium-review.googlesource.com/883533
Commit-Queue: Hugo Holgersson <hu...@vewd.com>
Reviewed-by: Fredrik Söderquist <f...@opera.com>
Cr-Commit-Position: refs/heads/master@{#589502}
---
A third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-search-beneath-exited-scroller.html
M third_party/blink/renderer/core/page/focus_controller.cc
M third_party/blink/renderer/core/page/spatial_navigation.cc
M third_party/blink/renderer/core/page/spatial_navigation.h
M third_party/blink/renderer/core/page/spatial_navigation_test.cc
5 files changed, 479 insertions(+), 64 deletions(-)
David Bokan (Gerrit)
2 comments:
File third_party/blink/renderer/core/page/spatial_navigation.cc:
LocalFrameView* root_frame_view = current_frame->LocalFrameRoot().View();
const LayoutRect root_doc_rect(
root_frame_view->GetScrollableArea()->VisibleContentRect());
// Convert the root frame's visible rect from document space -> frame space.
// For the root frame, frame space == root frame space, obviously.
LayoutRect frame_rect = root_frame_view->DocumentToFrame(root_doc_rect);
return frame_rect;The more straightforward way to get this is page->GetVisualViewport().VisibleRect()
Patch Set #30, Line 724:
visible_root_frameviewport_rect_in_root_frame
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.
Hugo Holgersson (Gerrit)
2 comments:
LocalFrameView* root_frame_view = current_frame->LocalFrameRoot().View();
const LayoutRect root_doc_rect(
root_frame_view->GetScrollableArea()->VisibleContentRect());
// Convert the root frame's visible rect from document space -> frame space.
// For the root frame, frame space == root frame space, obviously.
LayoutRect frame_rect = root_frame_view->DocumentToFrame(root_doc_rect);
return frame_rect;
The more straightforward way to get this is page->GetVisualViewport(). […]
Ack. I knew there was a shorter way! :) I could update this once I do a follow-up CL.
Patch Set #30, Line 724:
visible_root_frameviewport_rect_in_root_frame
Ack.
To view, visit change 883533. To unsubscribe, or for help writing mail filters, visit settings.