1448007 - does not fully scroll on GrabFocus call

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Samuel Thibault]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180317044808

Steps to reproduce:

- Open page http://dept-info.labri.fr/~thibault/tmp/bug-scroll.html
- Open accerciser
- Reach the objects corresponding to the last image showing up only partially at the bottom of the window (scroll a bit to the next image if it is actually showing up completely by pure luck of window size)
- in the IPython console, call acc.queryComponent().grabFocus()


Actual results:

- The targetted image gets the focus, but the window doesn't scroll to show it completely


Expected results:

- The window should scroll so that the image shows up completely.

That actually does happen if instead of targetting an image which is partially shown, one targets an image which is completely shown.

I will attached a patch fixing the issue

Comment 1

[Samuel Thibault]

Attachment: patch

Here is a proposed fix.

I looked a bit at the history: the flag was SCROLL_IF_NOT_VISIBLE as early as in hg revision 1 (i.e. 2007), and the SCROLL_IF_NOT_FULLY_VISIBLE flag seems to have been added only in revision 9cb4454dca34, i.e. 2012, so I tend to think that it was not on purpose that the scrolling does not happen when part of the element is already visible.

Comment 2

[Samuel Thibault]

It seems it disturbs some tests, notably bc7 browser_bug1184989_prevent_scrolling_when_preferences_flipped.js | Page should not scroll when search engineList flipped, but perhaps they just need to be fixed according to this new behavior.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddf6b431391cf5f02c0e20002516e127c1129f51

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8961394 [details] [diff] [review]
patch

Review of attachment 8961394 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks!

Comment 4

[Samuel Thibault]

I'm currently using the patch and do not see anything odd, but given the issues in tests, I'm really unsure about this.  In some cases it could be just a change of behavior which merely requires an update in the test. But for instance in editor/libeditor/tests/test_bug915962.html, the sanity check that win.scrollY is 0 after sc.focus() should probably not be updated: it does not make sense that firefox would scroll in that case. Perhaps there is a bug in the implementation of SCROLL_IF_NOT_FULLY_VISIBLE which brings spurious scrolls in some corner cases, which needs to be fixed first.

Given that we plan to add scrollto support to at-spi that screen readers could use in addition to grabfocus anyway, I'm not sure I really want to spend time on making nsFocusManager::ScrollIntoView use SCROLL_IF_NOT_FULLY_VISIBLE instead of SCROLL_IF_NOT_VISIBLE, given the potential unforeseen consequences due to bugs to be fixed first.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8961394 [details] [diff] [review]
patch

actually I'm not allowed to review the patch as it's out of scope of a11y, for some reason I was thinking about a11y's FocusManager version, when I was r+ the patch.

Comment 6

[alexander :surkov (:asurkov)]

cc'ing Neil

Comment 7

[Neil Deakin]

SCROLL_IF_NOT_FULLY_VISIBLE isn't quite correct. Some discussion is in bug 1278864 which this bug is probably a duplicate of.

Comment 8

[Samuel Thibault]

Mmm, indeed, the top-left behavior of SCROLL_MINIMUM probably gets in the way.  I agree that "focus() is not a method to bring an element into view", but it's still an odd behavior that when the element is completely out of view, scrolling happens, and if it's partially visible, no scrolling happens, while this is not what users get when they browse a page with tab or F7 caret browsing, etc.  So I'd say we could probably add another enum value which scroll by as little as possible to bring as much as possible of the frame into view?  I'd also argue that it is what would be mostly preferred to implement IA2's IA2_SCROLL_TYPE_ANYWHERE.

Comment 9

[Samuel Thibault]

James, perhaps you have an opinion on the behavior of IA2_SCROLL_TYPE_ANYWHERE? (or know someone who would have one)

Comment 10

[Samuel Thibault]

Mmm, diving into it again, it seems the scrolling already does happen.

The issue that remains, however, is that it does not scroll to show the whole image, but only to show the label of the image, which does not have the whole height of the image, but only of one line of text at the bottom of the image (that can be seen by clicking on the "label" object in accerciser, and increasing the font size of the webpage does change that height). The consequence is that if the image is below what is visible, the scroll does bring the whole image up since it brings the label which is at its bottom, but if the image is above what is visible, the scroll only brings the label into view, thus only a tiny bit of the image.

I guess we could find a way to make the frame of that label the same height as the image, I'll have a look.

Comment 11

[Samuel Thibault]

Ok, after more investigation, SCROLL_IF_NOT_FULLY_VISIBLE was indeed not correct when the object is too big for the scroll area. But that did get fixed in 184697371b6b for fixing bug #1416391 ("Fix Element.scrollIntoView in "nearest" mode to behave according to CSSOM spec. r=tnikkel"). That's why I said "the scrolling already does happen" above: it now works just correctly.

Now, concerning the scrolling of the whole image vs. the label of the image, this is because of the computation of useWholeLineHeightForInlines in PresShell::DoScrollContentIntoView(), which disables it when mWhenToScroll is SCROLL_IF_NOT_FULLY_VISIBLE. It seems odd to me to be conflating two things: when to scroll and by how much to scroll. This conflation takes back to before hg history, at a time when Where and When were conflated into "ANYWHERE", I didn't dive further.

Concerning IA2_SCROLL_TYPE_ANYWHERE, it is actually already using SCROLL_IF_NOT_FULLY_VISIBLE, and is thus probably affected by the image height vs label height issue.

So, to summarize, I'd say there are two things:

- separate out the useWholeLineHeightForInlines flag from "When" so one can use SCROLL_IF_NOT_FULLY_VISIBLE without odd image vs. label height.
- find out why using SCROLL_IF_NOT_FULLY_VISIBLE in nsFocusManager::ScrollIntoView brings unexpected scrolls in the testsuite

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.