836003 - Work - replace child selector in start-container with a broadcaster

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Allison Naaktgeboren :ally]

follow up for more efficient ui css as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=801000#c17

Comment 1

[Allison Naaktgeboren :ally]

Attachment: wip - works for snapped view

- I'm unsure about what to do with the start state of the broadcaster, as we can start in any viewstates. I wonder about the merits of embedding js to determine the value of MetroUtils.snapped, but that way could lie madness? 

- Matt, is this what you had in mind? 

- not finished (imho).
-- I think it would be smart to modify all the other consumers of stack[snapped=small] (the non snappedview specific ones) in addition to the start screens.
-- also modify the snapped=large consumers behave as expected. (I think that is meant to mean 'filled', but I have not examined fully)

Comment 2

[Allison Naaktgeboren :ally]

also note that 'case Ci.nsIWinMetroUtils.fullScreenLandscape:' was accidentially removed (I must have fat fingered it while cleaning out my debug code). I fixed it locally, but did not think it was worth uploading a new patch

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 708338 [details] [diff] [review]
wip - works for snapped view

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

Great!  This part looks fine, but I think you'll need a few additional changes (see below).

::: browser/metro/base/content/browser-ui.js
@@ +538,5 @@
>     */
>  
>    _adjustDOMforViewState: function() {
>      if (MetroUtils.immersive) {
> +      let aViewState = "";

Minor nit: 'a' prefix is for function arguments only.

@@ +543,2 @@
>        switch (MetroUtils.snappedState) {
> +          aViewState = "landscape";

Missing a "case" statement?

@@ +545,2 @@
>          case Ci.nsIWinMetroUtils.fullScreenPortrait:
> +          aViewState = "fullscreen";

Can we call this one "portrait" instead?

::: browser/metro/base/content/browser.xul
@@ +47,5 @@
>    <broadcasterset id="broadcasterset">
>      <broadcaster id="bcast_contentShowing" disabled="false"/>
>      <broadcaster id="bcast_urlbarState" mode="view"/>
>      <broadcaster id="bcast_preciseInput" input="imprecise"/>
> +    <broadcaster id="bcast_changedViewState" viewstate=""/> <!-- anaaktge - we can start in any state, this could be a problem?-->

We should add a call to _adjustDOMforViewState in BrowserUI.init.  That will ensure that this gets set to the correct value on startup.

@@ +248,5 @@
>  
>          <!-- Start UI -->
>          <hbox id="start-container" flex="1" class="meta content-height content-width" hidden="true">
>            <!-- portrait/landscape/filled view -->
> +          <hbox id="start" class="start-page" flex="1" observes="bcast_changedViewState">

You'll also need to make #toolbar-container observe this new broadcaster, and update some of the styles on it and its children that currently use #stack[snapped].

Comment 4

[Allison Naaktgeboren :ally]

Attachment: draft 0 - replacing descendant selectors with broadcasters

ended up being more entries than I realized.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 708867 [details] [diff] [review]
draft 0 - replacing descendant selectors with broadcasters

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

Looks good!

::: browser/metro/base/content/browser-ui.js
@@ +1210,5 @@
>        let section = window[sectionName];
>        if (section.init)
>          section.init();
>      });
> +    BrowserUI._adjustDOMforViewState(); // address win os viewstate

Since this is "private" to BrowserUI, you could consider calling it from BrowserUI.init instead.  Either way is fine with me, though.

Comment 6

[Allison Naaktgeboren :ally]

Moved to BrowserUI init

https://hg.mozilla.org/projects/elm/rev/e916ed42ee52