1047713 - [e10s] optgroup in <select> should not be selectable

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Alice0775 White]

Attachment: screenshot

optgroup in <select> should not be selectable.

STR
mouse up/down

Comment 1

[Alice0775 White]

Attachment: select.html

Comment 2

[Neil Deakin]

Attachment: menucaption

This patch fixes three bugs:

1. Adds a caption attribute for menuitems so that the captions can be distinguished and not selected. They also have to be disabled, otherwise I'd need to change all the native theme code to handle this.
2. Doesn't allow the checkmark on the captions.
3. Switches to check for the optgroup tag directly rather than the child count so that empty groups are handled properly.

Comment 3

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Neil, is this patch ready for review?

Comment 5

[Neil Deakin]

I'll update it and try it out.

Comment 6

[Neil Deakin]

Attachment: Updated patch

This patch fixes some issues with the selected index when reopening and adds a test

Comment 7

[Neil Deakin]

Attachment: Fix leak

Running the test causes a failure due to SelectParentHelper not being cleared.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Just to make sure I understand, what is leaked is the SelectParentHelper obj itself, right? And the prob could also be fixed by just importing it to a temporary obj instead of `this`?

Comment 9

[Neil Deakin]

Comment on attachment 8580015 [details] [diff] [review]
Updated patch

This adds a caption="true" for menuitems that are captions.

Comment 10

[Neil Deakin]

Comment on attachment 8580015 [details] [diff] [review]
Updated patch

This is used for <optgroup> headers. I'm not sure what should be used for accessibility. The caption is skipped in normal navigation, but the user may want to read it.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 8580015 [details] [diff] [review]
Updated patch

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

::: accessible/xul/XULMenuAccessible.cpp
@@ +116,5 @@
>  XULMenuitemAccessible::NativeInteractiveState() const
>  {
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::caption,
> +                            nsGkAtoms::_true, eCaseMatters)) {
> +    return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE;

it should be rather opposite, neither of states is expected, that's how we handle the HTML optgroup. Also it should expose roles::GROUPING (XULMenuitemAccessible::NativeRole())

could you please add a11y for tests it,
1) states into states/test_controls.xul
2) hierarchy into tree/test_combobox.xul

I don't see any examples, so if captioned menuitem doesn't contain its items as children then their group position should be fixed (you don't have to fix it in this bug).

r=me with comment addressed, thanks for fixing a11y!

Comment 12

[alexander :surkov (:asurkov)]

you know if it doesn't contain items as children then let's not change role for now because grouping containing no items is weird (let's have a follow up on it)

Comment 13

[[e-mail]]

(In reply to Neil Deakin from comment #9)
> This adds a caption="true" for menuitems that are captions.

Is there a good reason to use a menuitem for an optgroup? I'm thinking that the menu code would automatically skip the element if it was, say, a menucaption. (Of course you would want to bind the XBL and add it to the themes appropriately.)

Comment 14

[Neil Deakin]

(In reply to [e-mail] from comment #13)
> (In reply to Neil Deakin from comment #9)
> > This adds a caption="true" for menuitems that are captions.
> 
> Is there a good reason to use a menuitem for an optgroup? I'm thinking that
> the menu code would automatically skip the element if it was, say, a
> menucaption. (Of course you would want to bind the XBL and add it to the
> themes appropriately.)

I don't recall why now but I can try to create a separate element instead.

Comment 15

[Neil Deakin]

Attachment: Add a menucaption element

Comment 16

[Neil Deakin]

I'll do the accessibility part separately.

Comment 17

[[e-mail]]

Comment on attachment 8587420 [details] [diff] [review]
Add a menucaption element

>diff --git a/toolkit/themes/windows/global/menu.css b/toolkit/themes/windows/global/menu.css
I can't speak for the other themes, but our menu binding story is a mess.
We have a binding for menubar menus (and a separate binding for menubar menus with icons, although I don't think anybody uses them).
We have a binding for menupopup menus and menuitems, and a separate binding if they have an icon, a checkbox or a radio. Because those take up space, the regular binding for menuitems has different CSS applied to it to shift the menutext across.
We then use the icon binding for menuitems in menulists, but hide the area reserved for the icon.
Because you based the menucaption on the regular menupopup menuitem, it's getting native appearance which gives it an extra left margin.
Changing the menucaption's label to the iconic class seems to fix that margin but of course it makes the other styles not apply and I thought I might as well just give you f+ on the patch so far.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Neil, I was going through M6 bugs. What's the status here?

Comment 19

[Jim Mathies [:jimm]]

We just tested this on w3c schools and can't reproduce. There is a corner case where the index can be off and we select an opt group on first display, we're going to file a new bug on that.

Comment 20

[Jim Mathies [:jimm]]

Ok, after doing some more testing, we found this bug does exist when navigating using the keyboard. Reopening. However we do not think this blocks m6.

Comment 21

[Neil Deakin]

I think I should ask Neil what he thinks should be done here, based on comment 17. We can file a separate bug on cleanup of the menu bindings and theme.

I can make the menulist > menupopup > menucaption binding be closer to the iconic one. Or is there more?

Comment 22

[[e-mail]]

Ah, yes, I forgot to mention the resulting issue. At least on my Windows build, I get this display:
     RightJS
    RightJS 2.3.1
    RightJS 2.1.1
     Three.js
    Three.js r54
     Zepto
    Zepto 1.0rc1
     Enyo
etc. when the captions should obviously have no indentation, rather than extra indentation.

Comment 23

[Neil Deakin]

Attachment: Add a menucaption element, v2

Comment 24

[Neil Deakin]

Attachment: Add a menucaption element, v2.1

Comment 25

[Neil Deakin]

Neil, would you be able to review this again soon?

Comment 26

[[e-mail]]

Comment on attachment 8598602 [details] [diff] [review]
Add a menucaption element, v2.1

Sorry for the delay. This version seems to be fine, at least on Windows.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8afe52c600
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f0d3729d5f

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c8afe52c600
https://hg.mozilla.org/mozilla-central/rev/45f0d3729d5f

Comment 30

[Richard Marti (:Paenglab)]

I get this error:

Warning: Unknown pseudo-class or pseudo-element 'moz-any('.  Ruleset ignored due to bad selector.
Source file: chrome://global/skin/menu.css
Line: 240, Column: 24

Correct should be ':-moz-any()'

Comment 31

[Rezaul Huque Nayeem]

Reproduce on Firefox nightly 34.0a1 (2014-08-01) with windows 7 64 bit

The bug's fix is verified on latest Nightly 43.0a1 (Build ID : 20150820030202)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0

Comment 32

[Rakibul Islam Ratul]

Verified as fixed on latest aurora 42.0a2 (Build ID 20150911004112; User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0)

and also on latest nightly 43.0a1 (Build ID 20150911030204; User Agent Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0)

changing status resolved fixed to verified fixed.
[Testday-20150911]

Comment 33

[Khalid Syfullah Zaman [:khalid32]]

Reproduce on Firefox nightly 34.0a1 (2014-08-01) on ubuntu 14.04 LTS, 32-bit!

The bug's fix is verified on Nightly 44.0a1!

Build ID: 20151001030236
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20151002]