891056 - Defect - [8.1] Support "snapped view" in Windows 8.1

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P2)

Description

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:25.0) Gecko/20130707 Firefox/25.0
Build ID: 20130707031138

Tested  for iteration-9 on Windows 8.1 preview using latest nightly build from ftp://ftp.mozilla.org/pub/firefox/nightly/2013/07/2013-07-07-03-11-38-mozilla-central/

STR and expected result:
1.A Metro Firefox user launches Metro Firefox to the Awesome screen and swipes in the app bars.
2.Using the Windows standard gesture, the user puts Firefox into snapped view.
The user sees the app bars disappear, the Top Sites tile group's thumbnail tiles turn into standard tiles, and Bookmarks tile group and History tile group are moved from their location to the right of the Top Sites tile group to the location underneath it.
3.Using the Windows standard gesture, the user puts Firefox into fill view and full screen view.
4.The user sees the app bars return and the Awesome screen re-configured with the Bookmarks tile group and the History tile group to the right of the Top Sites tile group.

Actual result:
When I am doing step-2, "Using the Windows standard gesture, the user puts Firefox into snapped view", I see the app bars don't disappear.

Comment 1

[Samvedana (:Samvedana)]

For step-3, Windows 8.1 doesn't support fill view. We can adjust screen according to our needs.

"Windows 8.1 Preview does not have fixed-width view states. Users can now resize apps continuously down to a minimum width. (The default minimum width of an app is 500 pixels.) So apps no longer have the snapped and fill view states. Instead, you develop your app to be functional and good looking at any size down to the minimum" from http://msdn.microsoft.com/en-us/library/windows/apps/bg182890.aspx

Comment 2

[Jim Mathies [:jimm]]

(In reply to Samvedana (:Samvedana) from comment #1)
> For step-3, Windows 8.1 doesn't support fill view. We can adjust screen
> according to our needs.
> 
> "Windows 8.1 Preview does not have fixed-width view states. Users can now
> resize apps continuously down to a minimum width. (The default minimum width
> of an app is 500 pixels.) So apps no longer have the snapped and fill view
> states. Instead, you develop your app to be functional and good looking at
> any size down to the minimum" from
> http://msdn.microsoft.com/en-us/library/windows/apps/bg182890.aspx

Samvedana, would you mind filing a separate 8.1 specific bug on this, so we can concentrate on the 8.0 bug here with the app bars?

Comment 3

[Matt Brubeck (:mbrubeck)]

Let's use this bug for the 8.1 support.  This ties into a change I was already planning to make.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Let's use this bug for the 8.1 support.  This ties into a change I was
> already planning to make.

Do we still need a new bug on the 8.0 issue sgohil mentions in comment 0?

"When I am doing step-2, "Using the Windows standard gesture, the user puts Firefox into snapped view", I see the app bars don't disappear."

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #4)
> Do we still need a new bug on the 8.0 issue sgohil mentions in comment 0?

Comment 0 was about testing done on Windows 8.1 preview.  I don't see any Windows 8.0 issues mentioned in this bug.

Comment 6

[Matt Brubeck (:mbrubeck)]

p=3

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: clean up

Removes some unused CSS for #start-autocomplete (which doesn't exist anymore) and replace some JavaScript code with CSS.

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 796851 [details] [diff] [review]
part 1: clean up

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

Looks great and also fixes Bug 907935!

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: use resize events instead of Windows system messages

Comment 10

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 796984 [details] [diff] [review]
part 2: use resize events instead of Windows system messages

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

The start grid views are still depending on metro_viewstate_changed. With this change they can get out of sync with the windowState broadcaster.

I think we could still keep a viewstate changed event/broadcaster that gets sent by contentAreaObserver when it changes. We can then use it to update the views, hide the flyout panels and on StartUI instead of resize. Thoughts?

::: browser/metro/base/content/ContentAreaObserver.js
@@ +77,5 @@
>      return this._deckTransitioning;
>    },
>  
> +  get viewstate() {
> +    if (window.innerWidth < Services.prefs.getIntPref("browser.ui.snapped.maxWidth")) {

Nit: You can use the width/height properties defined in ContentAreaObserver.

@@ +292,5 @@
> +  _updateViewState: function() {
> +    let viewstate = this.viewstate;
> +    Elements.windowState.setAttribute("viewstate", viewstate);
> +    if (viewstate == "snapped") {
> +      FlyoutPanelsUI.hide();

This feels out of place here. We can move this logic to FlyoutPanelsUI if we have a viewstate changed event.

::: browser/metro/base/content/startui/StartUI.js
@@ +110,4 @@
>      }
>    },
>  
> +  get viewstate() {

Use this.chromeWin.ContentAreaObserver.viewstate instead.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 796851 [details] [diff] [review]
part 1: clean up

https://hg.mozilla.org/integration/fx-team/rev/c3b4537fb228

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c3b4537fb228

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: use resize events instead of Windows system messages (v2)

Addresses review comments.  Moves some shared view code into the "View" superclass.

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: Don't show overlay buttons in snapped mode

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: part 5: remove unused widget code

Note: I'm still finishing up part 4, which updates our tests.

Comment 16

[Brian R. Bondy [:bbondy]]

Comment on attachment 808971 [details] [diff] [review]
part 5: remove unused widget code

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

r+ assuming you're changing this code in other patches:
http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=snappedState&redirect=true

I noticed you had other patches but didn't check them myself.

::: widget/nsIWinMetroUtils.idl
@@ -51,5 @@
>  
> -  /**
> -   * Attempts to unsnap the application from snapped state to filled state
> -   */
> -   void unsnap();

I think it's the right thing to do to remove the ability for front end to try to do this since it has no effect on 8.1, even know it may be useful in 8.0.  Likewise I'm glad you left the MetroUtils code in place which is used by the filepicker so that it still works as it should on 8.0.

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: test updates

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=84656437a41f

Comment 18

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 808961 [details] [diff] [review]
part 2: use resize events instead of Windows system messages (v2)

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

/me claps
Looks great and works great. It will make testing snapped/portrait testing in desktop mode much easier too \o/. Thanks for taking this.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 808961 [details] [diff] [review]
part 2: use resize events instead of Windows system messages (v2)

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

Yeah looks good. Many +1s for easier testing and troubleshooting in -metrodesktop mode. Also, I figured it was a matter of time before View sprouted a constructor :) Eventually I'd like to get the various fetch items and populate grid view methods lined up so only the details of the different data providers need be in the individual views.

Comment 20

[Matt Brubeck (:mbrubeck)]

Landed on fx-team.  Note: I folded parts 2 and 4 together because they depend on each other.

https://hg.mozilla.org/integration/fx-team/rev/6fe9309ee090
https://hg.mozilla.org/integration/fx-team/rev/67ed5c8e786a
https://hg.mozilla.org/integration/fx-team/rev/eec73668438e

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/6fe9309ee090
https://hg.mozilla.org/mozilla-central/rev/67ed5c8e786a
https://hg.mozilla.org/mozilla-central/rev/eec73668438e

Comment 22

[Matt Brubeck (:mbrubeck)]

Attachment: rollup of parts 2-4 for Aurora approval

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Release of Windows 8.1

User impact if declined: Firefox UI renders incorrectly in split-screen mode in Windows 8.1, including significant usability problems with navigation toolbar and start screen.

Testing completed (on m-c, etc.): Landed on m-c on 9/24.

Risk to taking this patch (and alternatives if risky): This is a large patch for Metro; there is some risk of Metro regressions that we haven't caught yet.  We could wait for more testing on m-c if we want to minimize this.  The patch does not touch any non-Metro code, so there is no risk to desktop Firefox or other products.

String or IDL/UUID changes made by this patch:  None.  (I left out the "part 5" patch because it is not required on Aurora and it changed an IDL file.)

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 811271 [details] [diff] [review]
rollup of parts 2-4 for Aurora approval

Touches only Metro code and has been on central over a week - approving for Aurora so it can bake there too.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b375eef59f9

Comment 25

[Manuela Muntean [Away]]

Attachment: screenshot_Nightly.png

While testing this for iteration #16, with latest Nightly (build ID: 20131021030203), on both Win 8.1 32-bit and 64-bit, Metro in snapped view looks like in the attached screenshot, with the URL bar (navigation app bar) visible.

Isn't this expected?

Comment 26

[Manuela Muntean [Away]]

Attachment: screenshot_Aurora.png

I've encountered another issue:  with latest Aurora (build ID: 20131021004002), on both Win 8.1 32-bit and 64-bit, after making it the default browser, when entering in Metro mode, I'm stucked at the screen shown in this attachement.

Does anyone have any thoughts/suggestions? Thanks!

Comment 27

[Matt Brubeck (:mbrubeck)]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #25)
> While testing this for iteration #16, with latest Nightly (build ID:
> 20131021030203), on both Win 8.1 32-bit and 64-bit, Metro in snapped view
> looks like in the attached screenshot, with the URL bar (navigation app bar)
> visible.

Note that Windows 8.1 does not have "snapped" view like Windows 8 did.  We now use our special "snapped view" layout only when the window width is less than 600px.  ("Narrow view" might be a better name.)  Otherwise we use either our portrait or landscape layout, depending on the window width and height.

(In reply to Manuela Muntean [:Manuela] [QA] from comment #26)
> I've encountered another issue:  with latest Aurora (build ID:
> 20131021004002), on both Win 8.1 32-bit and 64-bit, after making it the
> default browser, when entering in Metro mode, I'm stucked at the screen
> shown in this attachement.

This usually means the browser is crashing on startup, or otherwise completely failing to start -- probably an unrelated bug in Aurora.

Comment 28

[Manuela Muntean [Away]]

> Note that Windows 8.1 does not have "snapped" view like Windows 8 did.  We
> now use our special "snapped view" layout only when the window width is less
> than 600px.  ("Narrow view" might be a better name.)  Otherwise we use
> either our portrait or landscape layout, depending on the window width and
> height.

Marking this verified based on Matt's comment from above and comment 25.

Comment 29

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #19 without some issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-11-27-03-02-01-mozilla-central/

- Ensured that "Full View" works and everything is being rendered correctly
- Ensured that the "50/50" view works and everything is being rendered correctly
- Ensured that the user can change the size of the window either left or right without any issues
- Ensured that there's no rendering issues when resizing the Firefox Metro view
- Ensured that the content in Firefox Metro is being displayed/rendered correctly when resized
- Ensured that you can use both Windows Desktop/Firefox Metro when using the side-by-side view (50/50)

Issues:

- When going through the different views available, some of the pages will be cut off under the awesome screen. Sometimes it also renders the page differently. Created Bug 944255