DuckDuckGo search layout is broken

VERIFIED FIXED in Firefox 21

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: roc)

Tracking

({regression, reproducible, verifyme})

Trunk
mozilla23
ARM
Android
regression, reproducible, verifyme
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 unaffected, firefox20 unaffected, firefox21+ fixed, firefox22 fixed, firefox23 verified, fennec21+)

Details

(Whiteboard: [webcompat], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Updated

6 years ago
Whiteboard: [webcompat]
(Reporter)

Updated

6 years ago
Component: General → Layout
Product: Firefox for Android → Core
(Reporter)

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox19: --- → unaffected
status-firefox21: --- → affected
Steps to reproduce?  Because I see nothing like what you see, on my Mac, even if I make the window very small.  Is this Android-specific?
tracking-firefox21: --- → ?
(Reporter)

Comment 2

6 years ago
Steps? Just load http://duckduckgo.com on an Android device. This seems Android specific, I don't see this on my FxOS (Unagi) device (b2g18-beta 02/14).

CC'ing :kats to see if he sees this too.
tracking-fennec: ? → 21+
I see this too on-device. Doesn't happen on desktop FF in responsive view. Regression range would be handy. I wonder if this was caused by the font change in Fennec.
(Reporter)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=954fc89373b8&tochange=7e9f3cc6cc0a

Looks like bug 833542, or 832788.
Keywords: regressionwindow-wanted → reproducible
(Reporter)

Updated

6 years ago
Blocks: 833542
Assignee: nobody → roc
tracking-firefox21: ? → +
(Reporter)

Updated

6 years ago
status-firefox22: --- → affected
(Reporter)

Updated

6 years ago
status-firefox20: --- → unaffected
Adding ni on :roc to help with next steps here,as this could be a regression from bug 833542, or 832788.
roc, any ideas here as this is a web compat issue and we do not know the number of sites impacted out there due to limited firefox mobile population we have on aurora/nightly ?
Flags: needinfo?(roc)
I don't know of any other sites impacted. Likely as not, duckduckgo just serves us something strange that depends on the bug we fixed in bug 833542.

I spent some time looking into this but didn't find a way to grab and save the content duckduckgo serves to mobile FF, so was unable to create a minimized testcase.
Flags: needinfo?(roc)
Keywords: qawanted
(Reporter)

Comment 7

6 years ago
For the position fixed footer issue also showing up on DuckDuckGo I filed bug 858181
Keywords: qawanted
Thanks.

The underlying problem is that scrollWidth on an element with zero height does not take the element's own width into account as it should. This is because its padding-box is a zero-height rectangle and is ignored by the union operations that build that scrollable overflow area.
Posted patch fix (obsolete) — Splinter Review
A better fix would be to make all ScrollableOverflow area calculations use UnionRectEdges. That's a much bigger change that we wouldn't want to take on branches. I've had a crack at it and it's not that hard, but I'll put it in a separate bug.
Attachment #734907 - Flags: review?(matspal)
Comment on attachment 734907 [details] [diff] [review]
fix

r=mats
Attachment #734907 - Flags: review?(matspal) → review+
Thanks, sorry.

The test failure revealed a real bug in the patch. The problem is that in GetScrollRectSizeForOverflowVisibleFrame, there are no children so nsLayoutUtils::UnionChildOverflow produces an empty rect 0,0,0,0, which we then combine with the frame's padding-rect using UnionEdges. When the padding-rect x,y is > 0, this extends the rectangle to include the point at 0,0. This is not what we want.
Posted patch fix v2Splinter Review
Attachment #734907 - Attachment is obsolete: true
Attachment #737927 - Flags: review?(matspal)
Comment on attachment 737927 [details] [diff] [review]
fix v2

r=mats
Attachment #737927 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/929fcbfd5618
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 737927 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 833542
User impact if declined: broken layout on DuckDuckGo
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): I think the risk is low because using scrollWidth/scrollHeight on non-scrollable elements with no children sounds very rare, and that's all that's affected here
String or IDL/UUID changes made by this patch: none
Attachment #737927 - Flags: approval-mozilla-beta?
Attachment #737927 - Flags: approval-mozilla-aurora?
Comment on attachment 737927 [details] [diff] [review]
fix v2

Request to mobile QA to help with verification here once this lands on branches.
Attachment #737927 - Flags: approval-mozilla-beta?
Attachment #737927 - Flags: approval-mozilla-beta+
Attachment #737927 - Flags: approval-mozilla-aurora?
Attachment #737927 - Flags: approval-mozilla-aurora+
Keywords: verifyme
https://hg.mozilla.org/releases/mozilla-aurora/rev/27f1091756b7
https://hg.mozilla.org/releases/mozilla-beta/rev/e3ff97187d14
status-firefox21: affected → fixed
status-firefox22: affected → fixed
status-firefox23: --- → fixed
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
You need to log in before you can comment on or make changes to this bug.