944255 - Defect - about:start bookmarks and history tiles cut off when using split views

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Currently when you view Firefox Metro in the side by side view (50/50), portions of "Top Sites", "Bookmarks" and "Recent History" will be cut off. If you increase the size of Firefox Metro and then decrease it back to 50/50, it will render the awesome screen different the second time around.

- Attached a video to illustrate the problem. You'll notice that when I initially slide in the Windows Desktop, some of the websites in the awesome screen are being cut off. When you resize and then return to the 50/50 view, you'll notice its being rendered differently.

Steps to reproduce the issue:

1) Open Firefox Metro (Windows 8.1)
2) Visit some websites to populate the categories (Top Sites, Bookmarks, Recent History)
3) Slide in the Windows Desktop and make the screen 50/50. You should notice that some of the websites under the awesome screen will be cut off
4) Slide the Firefox Metro to increase the window size
5) Once you made Firefox Metro bigger than Windows Desktop, return the views to 50/50. You'll notice that this time the "Top Sites" is being rendered differently

Current Behavior:

- When using the 50/50 view in Win 8.1, some of the websites are being cut off making it difficult to select them. The way the awesome screen is being rendered is also not a consistent experience

Expected Behavior:

- All of the sites under the awesome screen should be visible and easily selected. Firefox Metro should render the awesome screen the same way every single time making the user experience consistent throughout the browser.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

Attachment: issues with views in Win 8.1

Comment 2

[Kamil Jozwiak [:kjozwiak]]

Attachment: Views (cannot select pages that are cut off)

Comment 3

[Marco Mucci [:MarcoM]]

Hi Rodrigo, can you provide a point value.  Thanks.

Comment 4

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

p=2. Thanks.

Comment 5

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

Attachment: 944255.patch

Comment 6

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

Comment on attachment 8344077 [details] [diff] [review]
944255.patch

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

Looks good, just a couple of comments

::: browser/metro/modules/View.jsm
@@ +28,3 @@
>    this._set = aSet;
>    this._set.controller = this;
> +  this._window = aWindow;

You can also get the window reference with aSet.ownerDocument.defaultView, but I guess you're passing the ref in, in case element is orphaned at the time the constructor is called?

@@ +31,2 @@
>  
> +  this.onResize = () => this._adjustDOMforViewState();

why not put onResize on the prototype?

Comment 7

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

(In reply to Sam Foster [:sfoster] from comment #6)
> ::: browser/metro/modules/View.jsm
> @@ +28,3 @@
> >    this._set = aSet;
> >    this._set.controller = this;
> > +  this._window = aWindow;
> 
> You can also get the window reference with aSet.ownerDocument.defaultView,
> but I guess you're passing the ref in, in case element is orphaned at the
> time the constructor is called?
> 

Switched to using the defaultView. The grid shouldn't be orphaned, we call the view scripts after load and they should be destroyed before the window (I hope :)) 

> @@ +31,2 @@
> >  
> > +  this.onResize = () => this._adjustDOMforViewState();
> 
> why not put onResize on the prototype?

This was the simplest way to get the right this. Putting it as a function in the prototype would have the event target as this. I didn't want to add a handleEvent to the prototype because of the inheritance implications. Other option would be to do it similar to what we were doing for the observer by creating an object with a handleEvent method, but this was simpler.

Comment 8

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

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

Comment 9

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

Sounds and looks good.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-mc orange.
https://hg.mozilla.org/integration/fx-team/rev/d47a200fa99c

https://tbpl.mozilla.org/php/getParsedLog.php?id=31697557&tree=Fx-Team

Comment 11

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

Attachment: Test fix

Resize happens async after viewstate change and was causing the viewstate from previous test to linger intermittently. Waiting for Content:SetWindowSize:Complete fixed it locally.

try run: https://tbpl.mozilla.org/?tree=Try&rev=c3bcdc4c5148

Comment 12

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

Comment on attachment 8345162 [details] [diff] [review]
Test fix

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

I re-triggered a few times and we're all green. Nice catch.

Comment 13

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

Thanks for retriggering! Folded and landed again:

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

Comment 14

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

Adding [next-aurora-uplift] to whiteboard for uplift batching.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8dc8ac53473a

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8344077 [details] [diff] [review]
944255.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown

User impact if declined: Metro Firefox "Start" page has incorrect, unusable layout in certain window sizes.

Testing completed (on m-c, etc.): Landed on m-c on 12/11.

Risk to taking this patch (and alternatives if risky): Low-risk Metro-only fix.

String or IDL/UUID changes made by this patch: None.

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8345162 [details] [diff] [review]
Test fix

Test-only change that is required for the other patch in this bug.

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69a98ae06f1b

Comment 19

[Manuela Muntean [Away]]

Verified as fixed for iteration #21, with latest Nightly and Aurora, on Win 8.1 32-bit and 64-bit: in the side by side view (50/50), portions of "Top Sites", "Bookmarks" and "Recent History" aren't cut off.