Eitan Isaacson [:eeejay] Assignee
	Description • 10 years ago

Currently, the building block <select> styled elements use a transparency hack to style combo boxes. Besides being hackish, this makes accessibility problematic.

	Eitan Isaacson [:eeejay] Assignee
	Comment 1 • 10 years ago

I needed to extend HTMLSelectElement because the inputmethod API looks at the type of the event target. If the <select> is part of the shadow DOM it events get re-targeted, and we don't get inputmethod-contextchanged events.

	Kevin Grandon :kgrandon
	Comment 2 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Left some comments on github. Can you address and re-flag me? Thanks!

	Eitan Isaacson [:eeejay] Assignee
	Comment 3 • 10 years ago

Thanks,

I have been digging around more, and I can't really find a perfect alternative. I hope I answered you question in the pull request. I removed the inert CSS section.

	Kevin Grandon :kgrandon
	Comment 4 • 10 years ago

Eitan - what are your thoughts about putting the entire <select> element behind the shadow root of a custom element, would this work for accessibility? Basically I am trying to figure out what we can do to have a named custom element, like everything else inside the elements/ folder. I think it's important to be consistent here with the other components, so I would like to try to think of everything that we can do first before using the is="" property.

	Eitan Isaacson [:eeejay] Assignee
	Comment 5 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #4)
> Eitan - what are your thoughts about putting the entire <select> element
> behind the shadow root of a custom element, would this work for
> accessibility? Basically I am trying to figure out what we can do to have a
> named custom element, like everything else inside the elements/ folder. I
> think it's important to be consistent here with the other components, so I
> would like to try to think of everything that we can do first before using
> the is="" property.

There are two issues that make that hard:
1. Our IME code requires the event target to be an instance of HTMLSelectElement for us to receive the correct inputmethod-contextchanged event(i) that we use in the system app for value selector popups. If the <select> is in the shadow dom, the event gets retargeted to the root (<gaia-select>), and this doesn't work.
2. If gaia-select has children of <option> in the light DOM, the template for some reason doesn't pick them up (ii). I managed to clone them manually into the shadow DOM, but this inhibits us from being able to dynamically add and remove options (unless we add a mutation observer, and override add/remove).

There are workarounds for the issues above, but they come at a cost:
1. If I simply use the protocol of HTMLSelectElement, but don't use "extends", the instanceof test passes, and I could use <gaia-select>. If I call unoverridden methods (like add()), I get not implemented exceptions, so I need to re-implement all of HTMLSelectElement from scratch. I also wonder if this is possible because of a loophole in our web components implementation that will not always be available.
2. Like I said, we could add mutation observers and override add/remove, but is all that worthwhile?

i. http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#715
ii. http://jsfiddle.net/tmpyxy43/

	Kevin Grandon :kgrandon
	Comment 6 • 10 years ago

Thanks Eitan. So I really don't want to land stuff into gaia with the is="" attribute. I think that the <gaia-select> element should ship with a <label> like our switch and checkbox elements as well.

My preference here is to meet with some web component/accessibility guys to see what kind of platform support we could add to solve one of the top two issues in comment 5. What do you think? Can we needinfo some poeple in this bug, or hop on a Vidyo call with them? I think we are going to run into problems if certain elements have accessibility problems while in the shadow dom. It would be good to figure out a proper fix for this that allows us to be more flexible with placement.

	Eitan Isaacson [:eeejay] Assignee
	Comment 7 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #6)
> Thanks Eitan. So I really don't want to land stuff into gaia with the is=""
> attribute. I think that the <gaia-select> element should ship with a <label>
> like our switch and checkbox elements as well.
> 
> My preference here is to meet with some web component/accessibility guys to
> see what kind of platform support we could add to solve one of the top two
> issues in comment 5. What do you think? Can we needinfo some poeple in this
> bug, or hop on a Vidyo call with them? I think we are going to run into
> problems if certain elements have accessibility problems while in the shadow
> dom. It would be good to figure out a proper fix for this that allows us to
> be more flexible with placement.

The issues I mentioned in comment #5 are not accessibility related. Those are the problems standing in the way of making pretty syntax for gaia-select. I have a feeling that type extensions use is= for a reason, but yeah, we will need to chat with web component folks. Lets see what wchen says..

	William Chen [:wchen]
	Comment 8 • 10 years ago

For the first issue in comment #5, it may work if we used the event's originalTarget instead of target, which doesn't get retargeted, although that property isn't really specified.

For the second issue, <select> and <option> don't work that way in shadow DOM. The interaction between those two elements are defined in terms of their relationship in the DOM. Insertion points in the shadow DOM transform the rendered tree. The example you posted isn't going to work nor are there any plans to make something like that work, so we should not be using <option> as children of custom elements expecting it to behave similar to being a child of <select>.

One of the major use cases of the "is" attribute is to allow extension of special elements like "select" because it has a lot of special properties that would be hard or impossible to re-implement as a custom element.

I can see the desire to avoid the <select is="gaia-select> as it isn't as nice as <gaia-select>. If we really wanted to have a <gaia-select> that functioned similar to <select>, we could also introduce something like a <gaia-option> and use attached and detached callbacks to handle adding and removing the options on the <gaia-select>.

	Eitan Isaacson [:eeejay] Assignee
	Comment 9 • 10 years ago

Thanks! So in light of that, Kevin, what would be your preference?
1. Simply use is=, similar to my current PR, or..
2. Make a <gaia-select> which would require:
 (a) Making a <gaia-option> as well.
 (b) Modifying forms.jsm in gecko to use originalTarget.

I feel fine with either option.

	Kevin Grandon :kgrandon
	Comment 10 • 10 years ago

I still need to look further here to understand the issue, but will modifying forms.jsm allow us to use a <gaia-select> option? If so, I think I would prefer to try that.

I feel strongly that we should be using <gaia-select> instead of an is="" attribute. Although I don't really like the use of <gaia-option>, if we need to do that I think I would be ok with that. What do you think?

	Eitan Isaacson [:eeejay] Assignee
	Comment 11 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Added a blocker bug for the gecko inputmethod changes. I guess this is the syntax you wanted, so you must be super happy :)

The one last issue is labeling: Should this widget embed a label? For accessibility, that would be extremely nice. But I don't know if it would be stylable enough that folks would use it. Wondering if it should be a label element child?

	Kevin Grandon :kgrandon
	Comment 12 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Hey Eitan - If this is going to work well for you, I think I like this better.

I do think it would be good to be able to embed a label, like our other apps do. I think we will need to perform a careful dance, but we should be able to have a few options for styling the labels in the future, and once we have the necessary selectors in the platform, apps will be able to override this. For now I'd recommend doing this in a way where we could migrate the <select> elements in the settings app, unless you had some place in mind already that you wanted to migrate.

I assume we might be able to just include a nested <label>, like: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_switch/examples/index.html#L31

	Eitan Isaacson [:eeejay] Assignee
	Comment 13 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Radical change:

After introducing label, we ended up with this highly offensive markup:
<gaia-select>
  <label>Select a number</label>
  <option>One</option>
  <option>Two</option>
  <option>Three</option>
</gaia-select>

So instead, I opted to get rid of the shadow dom altogether, and go with something like this:
<gaia-select>
  <label>Select a number</label>
  <select>
    <option>One</option>
    <option>Two</option>
    <option>Three</option>
  </select>
</gaia-select>

The advantages over the current BB (aside from it being a clean break and backward compatible):
 1. Directly styling the select element with no opacity/button hacks.
 2. Associating the label with the select automatically.
 3. No need to modify forms.js in gecko.

Both of which are a clear win for a11y, but also clean markup, IMHO.

	Kevin Grandon :kgrandon
	Comment 14 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Thanks Eitan, I suppose I'm fine with the nested <select> box for now. It keeps the markup nice and tidy. I'm pretty happy with this, but I left a few comments on github. Could you address them and re-flag me? Thanks!

	Eitan Isaacson [:eeejay] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

You comments have been addressed (no more SVG). I ended up having to rework the auto-labeling, since HTMLLabelElement.control is readonly.

	Kevin Grandon :kgrandon
	Comment 16 • 10 years ago

Comment on attachment 8514474 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25667

Thank you for dealing with my complaining. I think this is awesome!

	Kevin Grandon :kgrandon
	Comment 17 • 10 years ago

In master: https://github.com/mozilla-b2g/gaia/commit/c910dc2d0625a896c7f731366ab91fa23c1a2664

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

sorry had to revert this for bustage like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=841613&repo=b2g-inbound starting with that checkin

	Kevin Grandon :kgrandon
	Comment 19 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #18)
> sorry had to revert this for bustage like
> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=841613&repo=b2g-
> inbound starting with that checkin

Hmm, I am fairly sure that we are blaming the wrong commit here. I'll look into this more though.

	Kevin Grandon :kgrandon
	Comment 20 • 10 years ago

Hey Carsten - I see this as being starred as you by being fixed by: https://hg.mozilla.org/integration/b2g-inbound/rev/3c903d82b482

Are we good to re-land here?

	Carsten Book [:Tomcat]
	Comment 21 • 10 years ago

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #20)
> Hey Carsten - I see this as being starred as you by being fixed by:
> https://hg.mozilla.org/integration/b2g-inbound/rev/3c903d82b482
> 
> Are we good to re-land here?

yeah since we identified the right one (sorry again) i guess we can reland

	Kevin Grandon :kgrandon
	Comment 22 • 10 years ago

Re-landed: https://github.com/mozilla-b2g/gaia/commit/6666835b6410eed0d1cbfef79806fe469b5f1571