841363 - DuckDuckGo search layout is broken

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (02/14) & Beta (Fx19) - Screenshot

See screenshot.

http://www.duckduckgo.com

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Aaron Train [:aaronmt]]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Aaron Train [:aaronmt]]

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

Looks like bug 833542, or 832788.

Comment 5

[bhavana bajaj [:bajaj]]

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 ?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Aaron Train [:aaronmt]]

For the position fixed footer issue also showing up on DuckDuckGo I filed bug 858181

Comment 8

[Aaron Train [:aaronmt]]

Saved DDG content over http://people.mozilla.com/~atrain/mobile/tests/DuckDuckGo.html
http://people.mozilla.com/~atrain/mobile/tests/DuckDuckGo_files/

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 734907 [details] [diff] [review]
fix

r=mats

Comment 12

[Cameron McCormack (:heycam)]

Original landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64abee9b00f1

Backed out as the likely cause of test_bug375003-1.html failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4828dd617269

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 737927 [details] [diff] [review]
fix v2

r=mats

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/929fcbfd5618

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/929fcbfd5618

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 737927 [details] [diff] [review]
fix v2

Request to mobile QA to help with verification here once this lands on branches.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27f1091756b7
https://hg.mozilla.org/releases/mozilla-beta/rev/e3ff97187d14