1351248 - Can't tab into or out of fieldset with overflow auto

Status: RESOLVED FIXED

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

Description

[User A]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

I tried to tab into and out of a fieldset with an active overflow.
------------------------------------------------------------

JSFiddle to reproduce:
https://jsfiddle.net/7pa3a9sv/2/
(Or use the HTML attached)

Steps (using FF 52):
1. Click on "This is div 1"
2. Press Tab key three times.

Alt steps:
1. Click into the second input field.
2. Press Shift-Tab three times.




Actual results:

Focus moved twice and then did not move any further forward (or backward, in Alt steps).




Expected results:

Focus should have moved into the first input field (or out of the input fields and onto the div, in the Alt steps).

- Issue does not occur without overflow on fieldset.
- Seen in FF 52.
- Not seen in Chrome or IE.

Comment 1

[User A]

This is three months old and still UNCONFIRMED, is there any update on when there might be movement or news on the issue?
Thanks

Comment 2

[User A]

This P3 is now nine (9) months old and FF is five versions advanced, but the bug is still in "UNCONFIRMED" state. 

I respectfully suggest it could quickly be confirmed by opening the provided JSFiddle in FF and comparing with other browsers. I believe my original report complies with the requirements here: 
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Confirming_unconfirmed_bugs
But please let me know if I can provide any other info.

This is one of several issues that may result in our team dropping FF support for our product, which I personally would hate to see happen.

Thanks
Al

Comment 3

[Andrew Overholt [:overholt]]

I think it's UNCONFIRMED because new accounts default to that state. It's also not super important what the state is :)

I used http://mozilla.github.io/mozregression/quickstart.html and it points to something that landed around 2013-10-24 or 2013-10-25 so it's not a new regression. I couldn't find duplicate bugs but perhaps I was just searching for the wrong terms. Given the age and the fact that it doesn't appear to be a widespread problem, it's hard to prioritize this higher than P3. I'll see what I can do, though.

mozregression is still chugging away trying to get a narrower set of landings and I'll comment back here when it finishes.

Comment 4

[Andrew Overholt [:overholt]]

smaug, do you see anything in https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-10-23&enddate=2013-10-25 that could have caused this? At first glance, *maybe* bug 602787?

Comment 5

[Andrew Overholt [:overholt]]

(mozregression went too far back to get taskcluster builds (or at least I think that's why it didn't point at a particular revision)))

Comment 6

[Boris Zbarsky [:bzbarsky]]

In that range, bug 261037 is the obvious thing: making "overflow: auto" not be a no-op on fieldsets in the first place.

Mats, do you have time to take a look at this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(This sounds more like layout-ish issue)

Comment 8

[Emilio Cobos Álvarez [:emilio]]

This is relatively straight-forward to fix.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1351248 - Make nsIFrame::IsFocusable not virtual. r=#layout-reviewers,smaug,edgar

The only override does nothing.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1351248 - Simplify nsIFrame::IsFocusable API. r=#layout-reviewers,smaug,edgar

We always compute the tabindex value, so just return it to the caller
all the time. This allows us to use early-returns which makes the code a
bit easier to follow.

This patch shouldn't change behavior.

Depends on D100422

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1351248 - nsFieldSetFrame should claim to be focusable if its inner scrollframe is. r=#layout-reviewers,smaug,edgar

Otherwise we stop moving focus because, when we find the scrollable
area, we determine that we need to focus the fieldset, but then the
focus manager early-returns in nsFocusManager::SetFocusInner.

Depends on D100423

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c1bddfacedd
Make nsIFrame::IsFocusable not virtual. r=smaug

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0c1bddfacedd

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df791c74125a
Simplify nsIFrame::IsFocusable API. r=layout-reviewers,mats
https://hg.mozilla.org/integration/autoland/rev/af18f24ef379
nsFieldSetFrame should claim to be focusable if its inner scrollframe is. r=smaug

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/df791c74125a
https://hg.mozilla.org/mozilla-central/rev/af18f24ef379