Closed Bug 944255 Opened 6 years ago Closed 6 years ago

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

Categories

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

28 Branch
All
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: kjozwiak, Assigned: rsilveira)

References

Details

(Whiteboard: [next-aurora-uplift][beta28] feature=defect c=Browser_views u=metro_firefox_user p=2)

Attachments

(4 files)

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.
Summary: Defect - Web pages on awesome being cut off in Win 8.1 50/50 view → Defect - Pages being cut off when using different views in Win 8.1
Summary: Defect - Pages being cut off when using different views in Win 8.1 → Defect - Pages cut off under awesome screen when using different views in Win 8.1
Whiteboard: feature=defect c=Browser_views u=metro_firefox_user p=0 → [triage] feature=defect c=Browser_views u=metro_firefox_user p=0
Whiteboard: [triage] feature=defect c=Browser_views u=metro_firefox_user p=0 → [release28] feature=defect c=Browser_views u=metro_firefox_user p=0
Summary: Defect - Pages cut off under awesome screen when using different views in Win 8.1 → Defect - about:start bookmarks and history tiles cut off when using split views
Assignee: nobody → rsilveira
Hi Rodrigo, can you provide a point value.  Thanks.
Flags: needinfo?(rsilveira)
p=2. Thanks.
Flags: needinfo?(rsilveira)
Whiteboard: [release28] feature=defect c=Browser_views u=metro_firefox_user p=0 → [release28] feature=defect c=Browser_views u=metro_firefox_user p=2
Whiteboard: [release28] feature=defect c=Browser_views u=metro_firefox_user p=2 → [beta28] feature=defect c=Browser_views u=metro_firefox_user p=2
Attached patch 944255.patchSplinter Review
Attachment #8344077 - Flags: review?(sfoster)
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
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?
Attachment #8344077 - Flags: review?(sfoster) → review+
(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.
Sounds and looks good.
Attached patch Test fixSplinter Review
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
Attachment #8345162 - Flags: review?(sfoster)
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.
Attachment #8345162 - Flags: review?(sfoster) → review+
Thanks for retriggering! Folded and landed again:

https://hg.mozilla.org/integration/fx-team/rev/8dc8ac53473a
Adding [next-aurora-uplift] to whiteboard for uplift batching.
Whiteboard: [beta28] feature=defect c=Browser_views u=metro_firefox_user p=2 → [next-aurora-uplift][beta28] feature=defect c=Browser_views u=metro_firefox_user p=2
https://hg.mozilla.org/mozilla-central/rev/8dc8ac53473a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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.
Attachment #8344077 - Flags: approval-mozilla-aurora?
Comment on attachment 8345162 [details] [diff] [review]
Test fix

Test-only change that is required for the other patch in this bug.
Attachment #8345162 - Flags: approval-mozilla-aurora?
Attachment #8344077 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8345162 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/69a98ae06f1b
Hardware: x86_64 → All
Version: unspecified → 28 Branch
Target Milestone: Firefox 29 → Firefox 28
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.