Closed Bug 616594 Opened 9 years ago Closed 9 years ago

overflow:auto causes DIV to get tab focus even if it has no scrollbars

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- -

People

(Reporter: ksv, Assigned: davidb)

References

Details

(Keywords: regression, Whiteboard: [d?])

Attachments

(4 files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

The DIV initially has a tabIndex=-1 by default (not explicitly set in markup).  Even though it has a tabIndex of -1, it will get tab focus if overflow has been set to auto in markup.  

You can workaround the issue by explicitly setting tabIndex=-1 in the markup.

Reproducible: Always

Steps to Reproduce:
1.Load the test html
2.It will use alert to identify the tabIndex.
3.Notice that you can tab to the "testing" div
Actual Results:  
The div will get tab focus.

Expected Results:  
The div should not get tab focus.
Attached file recreate tab bug
Version: unspecified → Trunk
This bug is causing all my automated tabbing tests to fail.
Still broken in beta 9
Still broken in beta 10
Still broken in beta 11
I've been able to recreate this bug in beta 11 on Mac OS X as well. I experimented with a variety of tags and, at very least, div and p tags are both affected. spans are not.

The introduction of additional tab stops into a page will significantly affect the user experience for non-mouse users such as those with screen readers.
Still broken in beta 12
Sorry for the delay. This bug is a bit worrisome.

Note to endgame drivers, fix is to revert:
http://hg.mozilla.org/mozilla-central/diff/93e95f4f73af/layout/generic/nsFrame.cpp
Status: UNCONFIRMED → NEW
status2.0: --- → ?
Component: Keyboard Navigation → Layout
Ever confirmed: true
Product: Firefox → Core
QA Contact: keyboard.navigation → layout
Whiteboard: [d?]
(See duped bug 636976 for additional discussion and test case)
blocking2.0: --- → ?
status2.0: ? → ---
So why is this a problem, exactly?  Why should it block release even if it is a problem?

Note that comment 0 is wrong: the default value of tabindex is NOT -1.  It has no default value.  The html5 spec at http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#sequential-focus-navigation-and-the-tabindex-attribute says:

  If the attribute is omitted or parsing the value returns an error
    The user agent should follow platform conventions to determine if the
    element is to be focusable and, if so, whether the element can be reached
    using sequential focus navigation, and if so, what its relative order
    should be.

Comment 7's experimentation with "a variety of tags" just shows that in CSS "overflow" does not apply to inlines.  If you style your <span> as display:block, you'll see the same behavior there.
Boris, the problem is that this can mean that focus lands on labels or divs. See the testcase on bug 636976, https://bugzilla.mozilla.org/attachment.cgi?id=515324. When you tab through the form, focus stops on the "password" label in addition to the actual password field.
Yes, but why is that a problem?  Depending on the font size and the window size, the "Password:" label might need to be scrolled to see the whole label, given the markup there.  So we focus it so that keyboard-only users can scroll it as needed.
The worst case scenario is a probably a puff-switch user having to tab through lots of divs unnecessarily. I think optimally we'd make it focusable only if it was truly user interactable at the time.

Note, I'm personally leaning towards .x+ for this bug given the lateness.

Doug, Colin, Katie, what is the worst impact you are aware of in the wild if we don't fix this bug for FF4?
According to Neil Deakin's 570275 comment 79, this would be a WONTFIX. So actually we'd need to build a case for fixing it. So again, finding actual impact.
blocking2.0: ? → -
I think the worst case scenario davidb describes is about right. It amounts to a serious usability problem for keyboard-only users and those with limited mobility. Styling an element with overflow:auto is not uncommon, so we will undoubtedly encounter this in the wild.

In response to Boris, I'm assuming it's a straightforward issue: if an element has overflow:auto AND it is currently showing scroll bars, it should be in the tab order. If scroll bars aren't showing, it shouldn't be focusable. Am I missing something? This also seemed to be the point in Neil Deakin's 570275 comment 79, if I understood it correctly.

The bug here is that elements without scroll bars are also focusable, and with  no apparent reason to the user. This was not the behaviour in previous versions of Firefox, and is inconsistent with every other browser I've tested with.
Colin, that's what bug 570275 comment 79 was about, yes.  As far as I can tell from that bug, the old behavior was causing problems....  In particular, a11y needed to know whether the element is focusable before we knew whether it had scrollbars, and there is no mechanism to deal with elements switching to and from being focusable as a result of layout changes (which can cause scrollbars to appear and disappear).
I want to investigate if we still need the a11y change that caused this sub-optimal behaviour.
TBPL: http://tbpl.mozilla.org/?tree=MozillaTry&rev=2acdfbef82ca
Assignee: nobody → bolterbugz
(In reply to comment #12)
> So why is this a problem, exactly?  Why should it block release even if it is a
> problem?
> 
> Note that comment 0 is wrong: the default value of tabindex is NOT -1.  It has
> no default value.  The html5 spec at
> http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#sequential-focus-navigation-and-the-tabindex-attribute
> says:
> 
>   If the attribute is omitted or parsing the value returns an error
>     The user agent should follow platform conventions to determine if the
>     element is to be focusable and, if so, whether the element can be reached
>     using sequential focus navigation, and if so, what its relative order
>     should be.
> 
> Comment 7's experimentation with "a variety of tags" just shows that in CSS
> "overflow" does not apply to inlines.  If you style your <span> as
> display:block, you'll see the same behavior there.

This comment is a little misleading.  Querying node.tabIndex of an INPUT element without an explicit tabIndex being set returns 0, but node.tabIndex of an overflow:auto DIV returns -1 and yet still gets focus.  Comment 0 was referring to this.  I think this is what makes the a11y change in question noncompliant with our autoamted testcase battery.
Yeah, tabindex="-1" isn't expected to put the node into the tab order. Also it is a common pattern to make the mouse hit target bigger than the actual widget contained within.
> but node.tabIndex of an overflow:auto DIV returns -1

That's a bug, per spec.  It should return 0.  See http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-tabindex

Is this filed?
I don't understand the part just before "This means that an element that is only focusable because of its tabindex attribute will fire a click event in response to a non-mouse activation (e.g. hitting the "enter" key while the element is focused)."

Why shouldn't clicking via the mouse fire the click event? Having mouse and keyboard use diverge here seems wrong to me.
David, clicking with the mouse always fires click events.  The text you quoted says that if something is only focusable because it has a tabindex explicitly specified and you tab to it and hit enter it should fire a click event, even though no mouse action actually happened.
Thanks Boris, I misinterpreted.

The WIP try server run ran clean with only known orange.
(In reply to comment #19)
> Created attachment 516875 [details] [diff] [review]
> WIP (investigating impact of revert)
> 
> I want to investigate if we still need the a11y change that caused this
> sub-optimal behaviour.

As I commented in bug 636976 a11y doesn't rely on this behavior anymore.
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

OK let's land a fix for FF5. Does the WIP look about right?
Attachment #516875 - Flags: review?(surkov.alexander)
This doesn't update things dynamically correctly when the scrollbars appear and disappear, right?  Or am I missing something?

Oh, and I'd like to be clear on why that change was made in the first place.
(In reply to comment #28)
> This doesn't update things dynamically correctly when the scrollbars appear and
> disappear, right?  Or am I missing something?

I'll test this.

> 
> Oh, and I'd like to be clear on why that change was made in the first place.

My assumption here is that during the bug 507275 work it was discovered that asking for the scrollbar info used to be done at an unsafe time but that is no longer the case. Alexander can you confirm?
Attachment #516875 - Flags: review?(surkov.alexander)
Attachment #516875 - Flags: review?(bzbarsky)
(In reply to comment #29)
> (In reply to comment #28)
> > This doesn't update things dynamically correctly when the scrollbars appear and
> > disappear, right?  Or am I missing something?
> 
> I'll test this.
> 
> > 
> > Oh, and I'd like to be clear on why that change was made in the first place.
> 
> My assumption here is that during the bug 507275 work it was discovered that
> asking for the scrollbar info used to be done at an unsafe time but that is no
> longer the case. Alexander can you confirm?

I don't think it was about scrollbars (but perhaps about scrollable areas, I don't recall exactly). And it wasn't about unsafe time, but we called this method "too early" (when nsCSSFrameConstructor::ContentRangeInserted calls into a11y) so that nsIFrame::IsFocusable returned false in some cases because layout isn't finished at this point. Now it's not an issue since we collect all this calls to proceed them after layout.
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

Thanks Alexander, I'm pretty happy how this tests out.
Attachment #516875 - Flags: review?(bzbarsky)
Attachment #516875 - Flags: feedback?(surkov.alexander)
I noticed I'm getting thebes asserts (even without a11y): 'Can only call this on frames that have been reflowed'. Doing a full rebuild to see what's up there.
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

OK the patch causes the assertion, and only when a11y is active. Cancelling reviews. (In other news I think I need an SSD)
Attachment #516875 - Flags: review?(bzbarsky)
Attachment #516875 - Flags: feedback?(surkov.alexander)
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

Gah! Further testing showed the assertions are intermittent and unrelated to this patch. Reloading this bug for example can cause many thebes reflow assertions or none, with and without this patch. Rerequesting review. Sorry for the noise. (I tweaked bug 627647 to show the assertions are not linux only)
Attachment #516875 - Flags: review?(bzbarsky)
Attachment #516875 - Flags: feedback?(surkov.alexander)
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

> but we called this method "too early" 

Please use IsZero() instead of reimplementing it.  Then you don't need the temporary (which if you do decide to keep it for some reason should be named something meaningful).
Attachment #516875 - Flags: review?(bzbarsky) → review+
Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

No other feedback from me is need I think since a11y doesn't rely on this. I saw failures in mochitest so if you don't get them now then we're on safe side.
Attachment #516875 - Flags: feedback?(surkov.alexander)
Attached patch patch to landSplinter Review
OK, thanks everyone. This addresses Boris's comment.
Flags: in-testsuite?
landed on cedar - http://hg.mozilla.org/projects/cedar/rev/f9acc7f1cc5a
Whiteboard: [d?] → [d?][fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/f9acc7f1cc5a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [d?][fixed-in-cedar] → [d?]
Target Milestone: --- → mozilla2.2
Duplicate of this bug: 647960
In what way is this fixed? The overflow focus behaviour still occurs in FF25.
The bug was about things that have overflow:auto but are not overflowing.  Those no longer get tab focus after the fix in this bug:

  data:text/html,<div style="overflow: auto; white-space: nowrap">Some text</div>

But if the div actually overflow, like this:

  data:text/html,<div style="overflow: auto; white-space: nowrap; width: 100px">Some text that is very long and just doesn't fit at all</div>

then the div of course has to take focus: it's the only way for a keyboard-only user to scroll it (by tabbing to it and then using arrow keys and the like).
Summary: overflow:auto causes DIV to get tab focus → overflow:auto causes DIV to get tab focus even if it has no scrollbars
Oh okay, thanks for the clarification.

However that being the case -- why is no additional behavior extended to the element? For example, tabbing to such an element, neither arrow keys nor the Space bar cause the element to scroll (testing on Mac 10.6 with Firefox 25).
My second testcase in comment 42 scrolls if I push the right arrow in Fx25 on Mac OS 10.8.
Hmm yes, that works for me, and still works if adjusted to overflow vertically instead.

But it's not working in the context I originally tested.  I'll see if I can isolate the difference and prepare a test case.
Okay, I don't understand why this would be, but -- if the page contains a content-editable region, then the arrow keys no longer scroll overflow elements:

data:text/html,<div style="overflow-x: auto; white-space: nowrap; width: 100px">Some text that is very long and just doesn't fit at all</div><p contenteditable="true">This is a content-editable region</p>
I would also venture to say that when Firefox implements this behavior, it should actually set a "tabindex" attribute on the element, so that it can be programatically removed in special cases (eg. when using roving tabindex).

As it is, the only way of taking it out of the tab-order is by setting tabindex "-1", however that's not the same as not having a tabindex at all -- elements with tabindex -1 can still be focused with the mouse, which is not the case for elements with no tabindex at all (even though the tabIndex property is -1 in both cases).

But if the tabindex attribute is set, then it can be removed with removeAttribute and tested with hasAttribute.
James, you can't have attribute values depending on computed values of CSS properties, much less on actual layout as here; that introduces cyclical dependencies.

The testcase in comment 46 seems to a bug to me.  I filed bug 936933.
Fair enough. So then how is the conflict I described to be resolved? Or is it just the case that elements with tabindex "-1" should not be able to take the focus at all?
I don't think I was very clear there, so let me clarify.

If Firefox has applied this behavior to an element, then it's no longer possible to prevent that element from taking focus, without using JS to control the event flow. That's do-able of course, but there's no way of determining that it's necessary, so you're left with no choice but to control the event flow on every element which MIGHT have this behavior.

I guess my broader contention is that: either elements with tabindex -1 should not be able to receive the focus at all, and/or the tabIndex property should reflect the difference between having no tabindex attribute, and having the value "-1" (perhaps returning null in the former case).

But every browser on every platform does the same thing, so is it a reportable bug, or just an unspoken convention?
James, you're right that there is no way to declaratively prevent focus on an element that is focusable by default right now.  That would include scrollable elements, but also textareas, inputs, links, etc.

It might be worth it to raise that issue as a spec issue for HTML.
Okay, thanks.
You need to log in before you can comment on or make changes to this bug.