Bug 52535 – offsetLeft is wrong for elements inside a TD whose style is set to position:relative

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jeremy Moskovich]

Chromium bug: http://crbug.com/15535

The attached test case contains a DIV inside a <TD stylel="position:relative"> .

The value returned for offsetLeft in the testcase by FF & IE is 302.
WebKit however, returns 2.

Removing position:relative from the TD causes the value of offsetLeft reported by FF/IE & WebKit to agree.

Browsers tested:
WebKit: r74962
FF 3.6.13
IE 8

From what I can tell, this bug renders the navigation menus in the following sites unusable:
http://www.doula.co.il
http://www.raanana.muni.il/
http://www.osem.co.il/_Contactus/Index.asp?CategoryID=12
http://www.beeper.co.il/?CategoryID=232
http://www.newpan.co.il/
http://www.water.gov.il/
http://www.most.gov.il/
http://www.ono.ac.il/
http://www.shoham.muni.il/
http://www.lehavim.muni.il/
http://www.givat-shmuel.muni.il/
http://www.qiryat-gat.muni.il/
http://www.oryehuda.muni.il/
http://www.efrata.muni.il/
http://www.tel-mond.muni.il/
http://www.kfar-yona.muni.il/
http://www.metar.muni.il/
http://www.gderot.muni.il/
http://www.golan.org.il/
http://www.qatzrin.muni.il/
http://www.beitberl.ac.il
http://www.michlalah.edu/
http://www.mishpat.ac.il/
http://www.arihav.com/products/sales/index.asp?category=tools

Possibly related WebKit bugs: 24305 and 26020

[Jeremy Moskovich]

Created attachment 79098 [details]
Testcase

[Jeremy Moskovich]

The code that calculates offsetLeft resides in RenderBoxModelObject::offsetLeft() [http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp#L389] .

Hyatt/Mitz/Eric: Can you vend me a clue on how I can tell if position:relative is set on the TD?  When I check style()->position() in this function it comes back as "static". 

I'm guessing that position:relative isn't valid for the TD element, so WebKit drops it on the floor? Since I need to check it's existence here to match FF/IE , any idea how I can tell that it's set?

[Levi Weintraub]

(In reply to comment #2)
> I'm guessing that position:relative isn't valid for the TD element, so WebKit drops it on the floor? Since I need to check it's existence here to match FF/IE , any idea how I can tell that it's set?

I'm not sure how you can tell if it's set or not, but I can verify that it's dropped on the floor in CSSStyleSelector.cpp::asjustRenderStyle:

// After performing the display mutation, check table rows.  We do not honor position:relative on
// table rows or cells.  This has been established in CSS2.1 (and caused a crash in containingBlock()
// on some sites).
if ((style->display() == TABLE_HEADER_GROUP || style->display() == TABLE_ROW_GROUP ||
     style->display() == TABLE_FOOTER_GROUP || style->display() == TABLE_ROW || style->display() == TABLE_CELL) &&
     style->position() == RelativePosition)
    style->setPosition(StaticPosition);


Perhaps we need a method to track whether we corrected away from the specified positioning in this case?

[Jeremy Moskovich]

Levi: Thanks, turns out I can access this by looking at the the raw style declaration in the node:
toElement(node())->style()->getPropertyCSSValue(CSSPropertyPosition)->cssText()

[Jeremy Moskovich]

Created attachment 83475 [details]
Work in progress

[Jeremy Moskovich]

Created attachment 83625 [details]
Work in progress

Now with layout test, not ready for review yet.

[Jeremy Moskovich]

After a bit more prodding it looks like the problem is actually in offsetParent() where the table element is skipped if the current element is relatively positioned.  Because WebKit overwrites the TD element's position CSS attribute, we don't correctly skip over the table element like other browsers.

[Jeremy Moskovich]

Created attachment 84273 [details]
Patch

[Jeremy Moskovich]

Reviewer note: another approach would be to add a flag to RenderObject or the style object to track that positon:relative was corrected away.  Unfortunately, from what I understand, growing RenderObject is frowned upon.

In short, if you think I should stash this info somewhere rather than looking at the raw style each time, please let me know where I should store the flag.

[James Robinson]

Would it be crazy to just support position:relative for table elements?

[Darin Adler]

Comment on attachment 84273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84273&action=review

Great to tackle this. This fix is wrong, though.

> Source/WebCore/rendering/RenderObject.cpp:2161
> +      // If relative positioning is already set, then no need to look at the
> +      // raw style.

Should put that comment on one line.

> Source/WebCore/rendering/RenderObject.cpp:2169
> +      CSSStyleDeclaration* rawStyle = toElement(node())->style();
> +      RefPtr<CSSValue> position = rawStyle->getPropertyCSSValue(CSSPropertyPosition);
> +      if (position.get() && position->cssText() == "relative")
> +          return true;
> +      return false;

This works only if the style is set with a style attribute, not when it’s set with a stylesheet. You should add a test case where position:relative is set in the stylesheet, perhaps based on class or ID, and that test case will fail.

If we need this information we will probably have to put it in the RenderStyle. Going back to the DOM doesn’t seem workable because there’s no practical way to go back to all the various style declarations that could have influenced the style of the element.

The style declaration that comes from the style attribute for an element is not called the “raw style” and coining that term here is not a good idea. That one style declaration contains only a small part of what determines the style of an element.

Should not need the call to “.get()” here. And if (x) return true; return false; could just be replaced by return.

Getting the cssText is not the right way to check a CSSValue. The right way is more like this:

    static bool valueIsKeyword(CSSValue* value, int keyword)
    {
        return value && value->isPrimitiveValue() && static_cast<CSSPrimitiveValue*>(value)->getIdent() == keyword;
    }

    ...

    return valueIsKeyword(rawStyle->getPropertyCSSValue(CSSPropertyPosition).get(), CSSPropertyRelative);

But I think that’s beside the point since this is the wrong approach.

> Source/WebCore/rendering/RenderObject.h:405
> +    // WebKit doesn't support relative positioning for certain elements.
> +    // In such cases isRelPositioned() won't match the value set in the original style declaration.
> +    // This function returns the value in the raw style declaration.

I think this is a confusing comment. It doesn’t make it clear if this lack of support is a bug or a feature. Same comment as above about the term “raw style”.

[Jeremy Moskovich]

Thanks for the review Darin!

The hack to override the style in CSSStyleSelector.cpp::adjustRenderStyle() was checked in at http://trac.webkit.org/changeset/10977 .  The commit says it's needed to fix a crash with http://cityoflakeforest.com/cs/pw/cs_pw2a3.htm .  While that URL is still live, the crash doesn't appear to reproduce anymore if I remove the workaround.

[Darin Adler]

(In reply to comment #12)
> The hack to override the style in CSSStyleSelector.cpp::adjustRenderStyle() was checked in at http://trac.webkit.org/changeset/10977 .  The commit says it's needed to fix a crash with http://cityoflakeforest.com/cs/pw/cs_pw2a3.htm .  While that URL is still live, the crash doesn't appear to reproduce anymore if I remove the workaround.

Since Hyatt made that change, I think he’s the best person to discuss it with.

[Jeremy Moskovich]

Thanks Darin, I'd be grateful if one of the cc-ed Apple folks could take a peek at rdar://4107882  and let me know if it notes any repro steps apart from the URL referenced in the CL description.

[Jeremy Moskovich]

ap was kind enough to look up the radar for me (thanks!), the only relevant comment is:
"""
The problem is an inline block with no containingBlock. I don't know whether this needs to be guarded against, or whether this is considered an illegal state.
""""

[Darin Adler]

Note that comment was by John Sullivan before our real expert on this, Hyatt, started working on the bug. Hyatt didn’t comment on his fix in the bug.

[Jeremy Moskovich]

Created attachment 84748 [details]
Patch

[Darin Adler]

Comment on attachment 84748 [details]
Patch

I would like Hyatt to review; rather than removing the code entirely there may be something else we need to do.

[Dave Hyatt]

Comment on attachment 84748 [details]
Patch

This wasn't a "hack."  Browsers don't support relative positioning of table cells.  You can't take this code out or you will cause compatibility problems.  We should look into fixing offsetParent in some way rather than changing this code.

[Dave Hyatt]

See

http://www.w3.org/TR/CSS2/visuren.html#choose-position

"The effect of 'position:relative' on table-row-group, table-header-group, table-footer-group, table-row, table-column-group, table-column, table-cell, and table-caption elements is undefined."

... which basically means "don't do it because browsers don't support it."

There is no technical limitation to supporting relative positioning on these objects, but historically browsers have not, since it would create bugs and compatibility issues to support it (since relative positioning changes both stacking and if used with left/top the actual placement).

[Dave Hyatt]

What we might have to do for this is add a bit like we did for positioning.  If isRelPositioned() uses a RenderObject bit, then it can not be set for the table types.  Then offsetParent could go back to the original style with a comment saying why it doesn't check the bit.

I'd really like to understand more about what other browsers do here though first.  You need to write a stacking test to see if IE and FFX are supporting position:relative layer creation.  Then a secondary test would be to set left/top on the table cell and see what happens.

Certainly at the time I implemented this change (many years ago), no browsers supported position:relative on table cells.  It may be that has changed.  Let's test and see.  Be sure to try quirks modes and strict mode when testing.

[Jeremy Moskovich]

Created attachment 84931 [details]
Patch

[Jeremy Moskovich]

Uploaded a new patch which only changes the behavior of offsetParent, the tracking bit is stored in StyleNonInheritedData .

[Darin Adler]

Comment on attachment 84931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84931&action=review

I wonder if there are any code paths that reuse the same style object and change the position. I couldn’t find any, but if they existed we’d want to clear this flag.

There might be some obscure case involving an SVG element.

> Source/WebCore/css/CSSStyleSelector.cpp:1939
> +            style->setPositionWasRelative(true);
>              style->setPosition(StaticPosition);

I noticed similar code for frame and frameset elements. Perhaps the bit should be set there too.

> Source/WebCore/rendering/RenderObject.cpp:2618
> -                    (!curr->isPositioned() && !curr->isRelPositioned() && !curr->isBody()))) {
> +                    (!curr->isPositioned() && !(curr->isRelPositioned() || curr->style()->positionWasRelative()) && !curr->isBody()))) {

It’s a little awkward here to use:

    !a && !(b || c) && !d

Instead of:

    !a && !b && !c && !d

Or:

    !(a || b || c || d)

If you instead added a single inline helper function that returned “is or was relative” then I could see keeping it the way you have it here.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
> +    // WebKit doesn't support position:relative for several table elements and overwrites the style internally when position:relative is encountered.
> +    // This flag preservers the fact that the position attribute was originally relative, and was subsequently "corrected".

Typo: “preservers”.

I’m not sure this comment strikes the right tone. The use of scare quotes around the word corrected and the words “doesn’t support” make it sound like the handling of relative positioning is a bug. Unless that is the case I would prefer not to give that impression.

[Jeremy Moskovich]

Created attachment 85163 [details]
Patch

[Jeremy Moskovich]

Thanks Darin, made all changes + fixed comments + added layout test for iframes.  Could you take another look please?

[Darin Adler]

Comment on attachment 85163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85163&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Jeremy  <[e-mail]>

Double change log.

> Source/WebCore/ChangeLog:34
> [phone]  Jeremy Moskovich  <[e-mail]>

Double change log.

> Source/WebCore/rendering/RenderObject.cpp:2155
> +bool RenderObject::isOriginallyRelPositioned() const

I would call this wasOriginallyRelativePositioned. No need to abbreviate, and the verb tense of "is" and "originally" don’t match.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
> +    // WebKit overwrites the position:relative style for certain elements for which such positioning is illegal or behavior is undefined.
> +    // This flag preserves the fact that the position attribute was originally specified as relative.

Don}t need this comment twice. I would suggest keeping it just in RenderObject.h.

[Jeremy Moskovich]

Created attachment 85298 [details]
Patch

[Jeremy Moskovich]

Created attachment 85299 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 85299 [details]
Patch

Clearing flags on attachment: 85299

Committed r80814: <http://trac.webkit.org/changeset/80814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dave Hyatt]

(In reply to comment #21)
> 
> I'd really like to understand more about what other browsers do here though first.  You need to write a stacking test to see if IE and FFX are supporting position:relative layer creation.  Then a secondary test would be to set left/top on the table cell and see what happens.
> 
> Certainly at the time I implemented this change (many years ago), no browsers supported position:relative on table cells.  It may be that has changed.  Let's test and see.  Be sure to try quirks modes and strict mode when testing.

I'm disappointed that my comments here were ignored by both the patch author and the patch reviewer.  It's possible we could have enabled relative positioning, but nobody even bothered to write tests to see what happens in IE9 or FFX4.

Also, a bit in the style isn't how I would have implemented this.  We already have RenderObject bits for this kind of separation for replaced, overflow, absolute/fixed positioning, etc.  If we're going to selectively ignore relative positioning, it would have been better to use the same approach as for other properties.

However, we still needed to do basic testing just to determine whether or not relative positioning really needed to remain off.

[Dave Hyatt]

Filed https://bugs.webkit.org/show_bug.cgi?id=56246 to track the investigation of relative positioning support.  As you can see from preliminary testing in that bug, this entire patch was probably unnecessary. :(

[Darin Adler]

OK, sorry Dave. My bad.

[Jeremy Moskovich]

Only half the sites listed in comment #1 are fixed by this patch, the other half have various other issues.

Sites which are still broken:
http://www.osem.co.il/_Contactus/Index.asp?CategoryID=12
http://www.beeper.co.il/?CategoryID=232
http://www.givat-shmuel.muni.il/
http://www.oryehuda.muni.il/
http://www.kfar-yona.muni.il/
http://www.metar.muni.il/
http://www.mishpat.ac.il/
http://www.michlalah.edu/
http://www.arihav.com/products/sales/index.asp?category=tools - side menus
http://www.tel-mond.muni.il/
http://www.golan.org.il/
http://www.qatzrin.muni.il/388/