Closed Bug 1015479 Opened 7 years ago Closed 7 years ago
Reorder gaia-header children to logical order
46 bytes, text/x-github-pull-request
|Details | Review|
Currently the markup order, and the visual order differ. Visually it is [action button][title][menu buttons], but in the markup it is [action button][menu buttons][title]. This is easily remedied with the new flex css!
I wonder if we should add a unit test which will validate the ordering of the elements?
Comment on attachment 8428137 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19613 Adding Arnau as he's better fitted to review the CSS than me.
Attachment #8428137 - Flags: review?(yor) → review?(arnau)
Comment on attachment 8428137 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19613 Eitan, the layout looks broken :( Please check: /shared/elements/gaia_header/examples/index.html I'll check tomorrow with Wilson if we could change the template, to remove the menu. That would fix your patch.
Attachment #8428137 - Flags: review?(arnau) → review-
IMO the structure of Shadow-DOM elements is meaningless. The Shadow-DOM is is meant to be purely structural so that the Light-DOM can become elegant and semantic. For example: screen-readers wouldn't parse the structural internals of the <video> element controls. My understanding is, Shadow-DOM is the internal/hidden implementation of a component and shouldn't ever be used to derive semantics.
Wilson, this is not the case. We need to expose the semantics of the shadow DOM elements to provide screen readers with the knowledge about the UI elements of the composit widget. For example, without parsing the shadow DOM of the video element, that video element would be nothing more than a blackbox. But the buttons you interact with like Play/Pause, Mute, the sliders and their information, need to be exposed to screen readers as well so the user knows if the video is playing, how far it has progressed, what volume it's at. The video element alone doesn't tell screen readers that. The same goes for web components. Information about those has to be exposed so the actual items to interact with or that provide other kinds of info can be made accessible. In fact, with the canvas element, the shadow DOM is the *only* way to make contents within it accessible.
Eitan, please change in script.js the template to: template.innerHTML = '<header id="header">' + '<button id="action-button"></button>' + '<content select="h1,button,a"></content>' + '</header>'; And will work the way you expect, as in your PR buttons in the menu appear one on top of the other. Using flex we don't need the menu anymore.
Comment on attachment 8428137 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19613 Updated the markup to your suggestion and removed the menu selector in the css.
Attachment #8428137 - Flags: review- → review?(arnau)
Comment on attachment 8428137 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19613 Much more flexible component now! Thanks Eitan.
Attachment #8428137 - Flags: review?(arnau) → review+
https://github.com/mozilla-b2g/gaia/commit/06e7fcde1627d741e06a32bce725f45ff2678953 Sorry for the delay, took travis a while to become green.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.