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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Katie Vance]

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.

Comment 1

[Katie Vance]

Attachment: recreate tab bug

Comment 2

[Doug Hays]

This bug is causing all my automated tabbing tests to fail.

Comment 3

[Doug Hays]

Still broken in beta 9

Comment 4

[Doug Hays]

Still broken in beta 10

Comment 5

[Doug Hays]

Still broken in beta 11

Comment 6

[Colin Clark]

Attachment: Here's another example document that recreates the bug with both div and p elements.

Comment 7

[Colin Clark]

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.

Comment 8

[Doug Hays]

Still broken in beta 12

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(See duped bug 636976 for additional discussion and test case)

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Marco Zehe (:MarcoZ)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 17

[Colin Clark]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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).

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP (investigating impact of revert)

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

Comment 20

[Doug Hays]

(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.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Boris, I misinterpreted.

The WIP try server run ran clean with only known orange.

Comment 26

[alexander :surkov (:asurkov)]

(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 27

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

(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?

Comment 30

[alexander :surkov (:asurkov)]

(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 31

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 516875 [details] [diff] [review]
WIP (investigating impact of revert)

Thanks Alexander, I'm pretty happy how this tests out.

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

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 33

[David Bolter [:davidb] (NeedInfo me for attention)]

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)

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

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)

Comment 35

[Boris Zbarsky [:bzbarsky]]

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).

Comment 36

[alexander :surkov (:asurkov)]

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.

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch to land

OK, thanks everyone. This addresses Boris's comment.

Comment 38

[alexander :surkov (:asurkov)]

landed on cedar - http://hg.mozilla.org/projects/cedar/rev/f9acc7f1cc5a

Comment 39

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f9acc7f1cc5a

Comment 41

[James Edwards]

In what way is this fixed? The overflow focus behaviour still occurs in FF25.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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).

Comment 43

[James Edwards]

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).

Comment 44

[Boris Zbarsky [:bzbarsky]]

My second testcase in comment 42 scrolls if I push the right arrow in Fx25 on Mac OS 10.8.

Comment 45

[James Edwards]

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.

Comment 46

[James Edwards]

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>

Comment 47

[James Edwards]

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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

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.

Comment 49

[James Edwards]

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?

Comment 50

[James Edwards]

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?

Comment 51

[Boris Zbarsky [:bzbarsky]]

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.

Comment 52

[James Edwards]

Okay, thanks.