The overflow property on an <iframe> is propagated to the viewport of the inner document (unlike other UAs)

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: videoclocknet, Assigned: mats)

Tracking

({compat, dev-doc-needed})

25 Branch
mozilla29
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(6 attachments, 3 obsolete attachments)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

Go to: http://materielmedicalmaroc.com/


Actual results:

The sliding bars don't appear. But you can navigate using the Arrow Keys, because some content needs sliding to be seen



Expected results:

The sliding bars have to appear
Posted image ie-sliding bars.png
The page:

-------------------------
<html>
<head>

</head>

<body style="margin:0px;padding:0px;overflow:hidden">
    <iframe src="http://www.abdellahnaim.com/medical" frameborder="0" style="overflow:hidden;height:100%;width:100%" height="100%" width="100%"></iframe>
</body></html>
-------------------------

With overflow:hidden, scroll bars should not appear.
I am not sure it should be scrollable though.
OS: Windows 7 → All
Summary: Sliding bar does not appear, but the user can use the Arrow Keys → No scrollbar, can scroll. iframe with "overflow:hidden;height:100%;width:100%" height="100%" width="100%">
Whiteboard: INVALID?
I can reproduce the issue mentioned in comment 0, with both Firefox 25 and latest Nightly (build ID: 20131201030203) on Win 8 64-bit.

Note: Chrome also shows the sliding bars.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → XUL
Product: Firefox → Core
Component: XUL → Layout
Posted file Testcase (obsolete) —
Posted file Testcase (obsolete) —
Attachment #8342123 - Attachment is obsolete: true
Attachment #8342125 - Attachment is obsolete: true
IE11 and Chrome33 show scrollbars, they don't propagate the 'overflow' property on the <iframe>
element to the viewport of the inner document.  I don't see anything in the spec for 'overflow',
or in HTML5, that supports our behavior.
Flags: needinfo?(dbaron)
Keywords: compat
Summary: No scrollbar, can scroll. iframe with "overflow:hidden;height:100%;width:100%" height="100%" width="100%"> → The overflow property on an <iframe> is propagated to the viewport of the inner document (unlike other UAs)
Whiteboard: INVALID?
<iframe> has a "scrolling" attribute.  We implement this by:

1)  Mapping it into style.
2)  Having the overflow style of the <iframe> propagate to the viewport inside.

The spec does require at http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-page that the "scrolling" attribute affect the viewport inside, but it does this via a direct hack.

I believe back when we implemented @scrolling (before any spec for this existed) we purposefully made it possible to achieve this purely presentational effect with just CSS without a content attribute.  It's not clear to me why the spec's proposed setup is better than ours in this case, though we could obviously implement it if needed.
I think it's worth raising the issue on appropriate lists (not sure whether whatwg or www-style) to see what others think before we match other browsers (and thus remove the possibility of changing in the other direction).
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #8)
> I think it's worth raising the issue on appropriate lists (not sure whether
> whatwg or www-style) to see what others think before we match other browsers

OK, could either of you do that please?
I think the HTML spec section that bz pointed out in comment 7 is probably the
best place to put this.

> (and thus remove the possibility of changing in the other direction).

We could still have that feature if we propagate the value of overflow-x/y
only if @scrolling is present with a yes/no value.  That would make us
compatible with other UAs when @scrolling is absent (fix this bug), while
still letting the author have full control over the inner viewport overflow
when needed.
I think we should just change our behavior to match the spec and other browsers. This feature is quite unimportant IMHO and it appears we're breaking real sites with our behavior.
Posted file Testcase
It appears the problem is actually worse than I first thought.
scrolling="yes" maps to 'overflow:scroll' in Firefox and it overrides
any specified 'overflow' in the sub-document.  In IE11/Chrome33,
scrolling="yes" has exactly the same behavior as not setting the
attribute at all, afaict.
Assignee: nobody → matspal
Posted patch fixSplinter Review
* Remove the MapAttributesIntoRule stuff for @scrolling.
* Map @scrolling and pass that to FrameLoader::Show.
* Add nsGenericHTMLFrameElement::AfterSetAttr to catch dynamic changes,
schedule a style change reflow when changed.

This makes us compatible with IE/Chrome afaict.
Attachment #8350997 - Flags: review?(bzbarsky)
Posted patch reftests (obsolete) — Splinter Review
Comment on attachment 8350997 [details] [diff] [review]
fix

>+nsGenericHTMLFrameElement::MapScrollingAttribute(const nsAttrValue* aValue)

This should be looking for the on/scroll/yes values as well, shouldn't it?  That's what our old code did and what the spec says to do.  Please add tests to make sure this doesn't get broken again?

>+nsGenericHTMLFrameElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,

>+  if (aName == nsGkAtoms::scrolling && aNameSpaceID == kNameSpaceID_None) {
>+    if (mFrameLoader) {

Please combine the conditions and outdent.

r=me with both of those fixed.
Attachment #8350997 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #15)
> >+nsGenericHTMLFrameElement::MapScrollingAttribute(const nsAttrValue* aValue)
> 
> This should be looking for the on/scroll/yes values as well, shouldn't it? 

It starts with:

  int32_t mappedValue = nsIScrollable::Scrollbar_Auto;

So for on/scroll/yes we'll just fall through and return that.
Same for bogus values or non-existent attribute.

This is what I intended - I can add a code comment to make that
intention clear if you think it's needed.


> >+nsGenericHTMLFrameElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> 
> >+  if (aName == nsGkAtoms::scrolling && aNameSpaceID == kNameSpaceID_None) {
> >+    if (mFrameLoader) {
> 
> Please combine the conditions and outdent.

Actually, that separation is intentional.  I think these methods
(Before/After//SetAttr etc) that does something for a specific
attribute should be written like this:

if (aName == nsGkAtoms::attr1 && aNameSpaceID == kNameSpaceID_None) {
  if (mFrameLoader)
    ...
} else if (aName == nsGkAtoms::attr2 && aNameSpaceID == kNameSpaceID_None) {
  if (mWhatever)
    ...
} else if (aName == nsGkAtoms::attr3 && aNameSpaceID == kNameSpaceID_None) {
  ...
}

This is so that when aName == attr1 we don't want to do all the
remaining attrN checks if mFrameLoader happens to be null.
Performance is a minor concern, but it also makes the code easier
to understand / less error prone IMO.

Do you still want me to change it?
Flags: needinfo?(bzbarsky)
Posted patch reftestsSplinter Review
Changed a couple scrolling=no to use off/noscroll, and a couple of
scrolling=yes to on/scroll just in case we don't have any tests on that.
Attachment #8350998 - Attachment is obsolete: true
> So for on/scroll/yes we'll just fall through and return that.

Yes, but we shouldn't be using auto scrollbars in that case, per spec.  If you think the spec is wrong, why do you think that and have you raised a spec issue?

> Actually, that separation is intentional.

OK, fair.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #18)
> Yes, but we shouldn't be using auto scrollbars in that case, per spec.

I agree that's what the spec currently says.

> If you think the spec is wrong, why do you think that

It's clear that IE/Chrome/Safari does NOT implement what the spec says
and some sites expect their behavior and thus breaks in Firefox.  IMO,
we should change our implementation to match the other UAs, and have
the spec changed accordingly.

> and have you raised a spec issue?

Not yet, but I can post a request for that spec change on
whatwg@whatwg.org first if you want.
Flags: needinfo?(bzbarsky)
Sites expect the auto behavior for scrollbars="yes"?

I'm probably OK with matching the other UAs as long as you raise a spec issue, yes.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #20)
> Sites expect the auto behavior for scrollbars="yes"?

Fair enough, I have no evidence of that, but I think it's more likely
given the behavior of other UAs.  I'm rather pessimistic about our
chances of convincing other UAs to implement the scrolling=yes to
overflow:scroll mapping at this point, for that reason.

Note that we should request a spec change on the "presentational
hint" part anyway, because no UA implements that as far as I can tell,
including Firefox (both before and after this patch).

The spec says that the mapped overflow value is a presentational
hint ("author-level zero-specificity"[1]), which I interpret has lower
specificity than any author rules[2] in the sub-document, but all UAs
make the mapped value trump styles in the sub-document, even !important.
Please correct me if I misunderstood the spec(s) here.

[1]
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#presentational-hints
[2]
http://www.w3.org/TR/css3-cascade/#preshint
You understood correctly.  I'm a little surprised we don't do that; it's impossible to override the "default scrollbar preferences"?
Yes, it appears so.  It's pretty clear in the code too:
http://hg.mozilla.org/mozilla-central/annotate/cf2d1bd796ea/layout/generic/nsGfxScrollFrame.cpp#l2493
Scrollbar_Never/Scroll overwrites the passed in aValue.

Fwiw, I tested Firefox 3.6 on Linux and it behaves the same.
We need to update the documentation for @scrolling here:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-scrolling
and here:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/frame#attr-scrolling

Might be worth waiting a few more days on a response from whatwg@ though...
Keywords: dev-doc-needed
Whiteboard: [DocArea=CSS]
Depends on: 1015986
You need to log in before you can comment on or make changes to this bug.