1230665 - [flexbox][css-grid] Can't tab to children after an anonymous item inside a flex/grid element with tabindex

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[alexlande]

Attachment: Reduced Test Case

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.73 Safari/537.36

Steps to reproduce:

I can reproduce this issue consistently in the latest Stable (42.0) and Nightly (45.0a1 (2015-12-04) for OS X. A reduced test case is available here: http://output.jsbin.com/peyujo/3/. The attached file includes the content of the JSBin for convenience. To reproduce:

1. Open the reduced test case.
2. Press the Tab key three times to attempt to tab to the `button` labeled "Broken".


Actual results:

The browser focuses on the `div` element with `tabindex="0"`, then the first `button` (labeled "Works"), then the `div` element with `tabindex` again.


Expected results:

The browser focuses on the `div` element with `tabindex="0"`, then the first `button` (labeled "Works"), then the second `button` (labeled "Broken").

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
20151205030208

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame tree

Comment 3

[Mats Palmgren (inactive)]

The problem is that as we're traversing the frames to find the next tabbable thing
(nsFocusManager::GetNextTabbableContent) we' find to the :-moz-anonymous-flex-item
frame which has the same content as the flex container which has tabindex=0, which is
tabbable, so we cycle between the button and the flex container...

This doesn't happen display:grid since we don't wrap a placeholder in a grid item
there, but adding a bit text after the button to force a :-moz-anonymous-grid-item
frame, the same thing happens in a grid container.

We need to make those pseudo frames non-tabbable somehow.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

Something like this should fix it I think.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix+test

Only tested locally on Linux so far, but I'll make sure it passes Try
before landing.

Comment 6

[Mats Palmgren (inactive)]

Updating subject since it's a more general bug than stated.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8696545 [details] [diff] [review]
fix+test

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

::: dom/html/test/test_bug1230665.html
@@ +13,5 @@
> +  document.getElementById("flexbutton1").focus();
> +  synthesizeKey("VK_TAB", { });
> +  var e = document.getElementById("flexbutton2");
> +  is(document.activeElement, e, "focus in flexbutton2 after TAB");
> +  

trailing whitespace

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8696545 [details] [diff] [review]
fix+test

This caused a lot of test failures on Try, so it needs more work.
It seems it broke focus handling for <input type=number> which uses
flexbox layout internally.  E.g. reftest that failed:
layout/reftests/forms/input/number/focus-handling.html

Comment 9

[Mats Palmgren (inactive)]

FTR, here's the frame tree for a <input type=number> :

NumberControl(input) ... [content=7fe497753db0] [sc=7fe4a055f1e0]<
  FlexContainer(div) ... [content=7fe49f305790] [sc=7fe4a055fc60:-moz-number-wrapper]<
    nsTextControlFrame ... [content=7fe497754570] [sc=7fe4a05bd2e0:-moz-number-text]<
      HTMLScroll(div) ... [content=7fe49f305af0] [sc=7fe4a05bdbb0]<
        Block(div) ... [sc=7fe4a05be208:-moz-scrolled-content]<
          Text(0)"" ... [sc=7fe4a05be7a8:-moz-non-element^7fe4a05bdbb0^7fe4a05bd2e0^7fe4a055fc60]
        >
      >
    >
    FlexContainer(div) ... [content=7fe49f305940] [sc=7fe4a05bea68:-moz-number-spin-box]<
      Block(div) ... [content=7fe49f3059d0] [sc=7fe4a05e5068:-moz-number-spin-up]<
      >
      Block(div) ... [content=7fe49f305a60] [sc=7fe4a05e5768:-moz-number-spin-down]<
      >
    >
  >
>

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix+test, v2

Check explicitly for the two pseudo classes we want to exclude.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4130e4cc0de4

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91d546e91e8a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91d546e91e8a