Closed Bug 836003 Opened 8 years ago Closed 8 years ago

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


(Firefox for Metro Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: ally, Assigned: ally)



(Whiteboard: feature=work [completed-elm])


(1 file, 1 obsolete file)

follow up for more efficient ui css as suggested in
Summary: replace child selector in start-container with a broadcaster → Work - replace child selector in start-container with a broadcaster
Whiteboard: feature=work
Attached patch wip - works for snapped view (obsolete) — Splinter Review
- 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)
Assignee: nobody → ally
Attachment #708338 - Flags: feedback?(mbrubeck)
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 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].
Attachment #708338 - Flags: feedback?(mbrubeck) → feedback+
ended up being more entries than I realized.
Attachment #708338 - Attachment is obsolete: true
Attachment #708867 - Flags: review?(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.
Attachment #708867 - Flags: review?(mbrubeck) → review+
Moved to BrowserUI init
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: feature=work → feature=work [completed-elm]
Blocks: 831894
You need to log in before you can comment on or make changes to this bug.