875060 - Image issues with apple.com

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Anthony Scotti]

Attachment: issue.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130522 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130522031027

Steps to reproduce:

Go to Apple's OS X page at 'http://www.apple.com/osx/'


Actual results:

The images used for the scroll effect on top of the page are not being partially hidden.


Expected results:

It should be rendered the same as Firefox 21 and other browsers. This is issue is also in Firefox 23.

I do not know the underlying issue or if it's something that just relates to apple.com but there's differences between versions of Firefox rendering the page.

Please see images for more details.

Comment 1

[Anthony Scotti]

Attachment: Screenshot from Firefox 24

Comment 2

[Anthony Scotti]

Attachment: Screenshot from Firefox 23

Comment 3

[Anthony Scotti]

Attachment: Screenshot from Firefox 21

Comment 4

[Anthony Scotti]

Attachment: Screenshot from Chrome 27

Comment 5

[Anthony Scotti]

Attachment: Screenshot from Opera 12.15

Comment 6

[Markus Stange [:mstange]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=55f9e3e3dae7&tochange=768af8d8fad4

Bug 841192 is in that range so I'm blaming it.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like this is over to you, roc.

Comment 9

[Alice0775 White]

Attachment: reduced zip (open index.html)

Comment 10

[Timothy Nikkel (:tnikkel)]

It looks like overflow: hidden display: -moz-inline-stack frames would clip position: absolute descendants. But bug 841192 fixed this. Was this an intentional quirk of moz-inline-stack? It appears to have been noticed by someone at least. Not sure how common exploiting this is on the web.

The site has

  @-moz-document url-prefix() { 
    .gallery-view { position: absolute; }
  }

to apply the position absolute, and then also uses -moz-inline-stack. If you remove both of these gecko specific hacks the site renders fine. So it looks at though they added one gecko specific hack to work around a bug in another gecko specific hack in their site.

Comment 11

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

I don't think we should do anything to work around this.

Comment 12

[Markus Stange [:mstange]]

(In reply to Timothy Nikkel (:tn) from comment #10)
> It looks like overflow: hidden display: -moz-inline-stack frames would clip
> position: absolute descendants.

Note that the element additionally has position:relative set on it, which really should have the effect of clipping position:absolute descendants. display:-moz-inline-stack now seems to revert the effect.

Comment 13

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

OK, then we still have a bug to fix here.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange from comment #12)
> (In reply to Timothy Nikkel (:tn) from comment #10)
> > It looks like overflow: hidden display: -moz-inline-stack frames would clip
> > position: absolute descendants.
> 
> Note that the element additionally has position:relative set on it, which
> really should have the effect of clipping position:absolute descendants.
> display:-moz-inline-stack now seems to revert the effect.

Oh, the reduced testcase didn't have that. So I guess it is for a slightly different change.

Comment 15

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

The basic problem here is that relatively-positioned -moz-inline-stacks don't make abs-pos containers.

Comment 16

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

Bug 841192 altered this because prior to that fix, CSS 'overflow' clipping on -moz-inline-stacks would clip all the element's descendants, not just the ones for which it's the containing block. This is because nsStackFrame::BuildDisplayListForChildren forces all the display items for the element's descendants into the "Content" list, which is always clipped by nsXULScrollFrame's OverflowClip.

After 841192 was fixed, a XUL element behaves just like a normal element and only clips descendants for which it is the containing block. That, plus comment #15, means they don't clip abs-pos descendants.

Comment 17

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

I think the best approach here would be to restore the behaviour of -moz-inline-stack/-moz-stack so they clip all their descendants if overflow:hidden. Making them honour position:relative would also work but is probably too risky. The extra overhead can be almost entirely confined to XUL-related code.

Comment 18

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

Attachment: fix

Adding code to handle something this obscure sucks in a way. A better thing to do might be to disable -moz-inline-stack and -moz-stack in Web content. However that's risky and we need a regression fix here.

Comment 19

[Mats Palmgren (inactive)]

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

layout/generic/nsGfxScrollFrame.h
>+  // True if we should clip all descendants, false if we should only clip
>+  // descendants for which we are the containing block
>+  bool mClipAllDescendants;

Please add :1 as the other bool members.

Nit: end the comment with a period.

Please also file a follow-up evangelism bug to make Apple.com stop
using -moz-* hacks like this since we intend to disable them for
web content (soon I hope).

Comment 20

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

(In reply to Mats Palmgren [:mats] from comment #19)
> Please also file a follow-up evangelism bug to make Apple.com stop
> using -moz-* hacks like this since we intend to disable them for
> web content (soon I hope).

We don't have to. Just ignoring the -moz-inline-stack rule makes the site work fine.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2351cb2579

Comment 22

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 841192
User impact if declined: broken rendering on apple.com page
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low risk, confined to users of XUL CSS feature
String or IDL/UUID changes made by this patch: none

Comment 23

[Ed Morley [:emorley]]

Push backed out for reftest failures:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff486a6eb5c9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad2d8b5e7c9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8048b030fe6

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f2d55d17a0d8

Comment 24

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

Looks like this busted layout/reftests/bugs/411585-1.html.

Comment 25

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

It wasn't this, it was bug 876092.

Comment 26

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/db02f2b140d2

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift nomination for Beta?

Comment 29

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 841192
User impact if declined: slightly broken rendering on Apple.com
Testing completed (on m-c, etc.): landed on m-c for quite some time
Risk to taking this patch (and alternatives if risky): low risk. Only affects -moz-specific properties that aren't useful on the Web
String or IDL/UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f8db9a9b1de

Comment 31

[Mihai Morar, (:MihaiMorar)]

I confirm this is fixed on FF 23b5 on Mac OS 10.9:

Build ID: 20130711122148

Comment 32

[Mihai Morar, (:MihaiMorar)]

(In reply to Mihai Morar, QA [:MarioMi] from comment #31)
> I confirm this is fixed on FF 23b5 on Mac OS 10.9:

Verified Fixed on Win 7 x64 and Ubuntu 13.04 x64 too on same FF23b5

Comment 33

[Mihai Morar, (:MihaiMorar)]

Verified Fixed on FF 24b1 too.
BuildID: 20130806104538