Bug 110349

Summary: [css exclusions] overflow:hidden undoes shape-outside offsets
Product: WebKit Reporter: Bear Travis <betravis>
Component: CSSAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: bjonesbe, eric, esprehn+autocc, gtk-ews, ojan.autocc, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://codepen.io/btravis/pen/HuecB
Bug Depends on:    
Bug Blocks: 98664    
Attachments:
Description Flags
Patch
jchaffraix: review+, jchaffraix: commit-queue-
Patch none

Description Bear Travis 2013-02-20 07:32:21 PST
Setting overflow:hidden, float: left, and shape-outside: rectangle does not offset correctly.
See link for an example.
Comment 1 Bem Jones-Bey 2013-03-18 15:21:20 PDT
It looks like the shape is still handled normally, but the offset for the content of the float is no longer handled properly when overflow is hidden. Still very strange, but it is comforting that the wrapping behavior doesn't change in this case.
Comment 2 Bem Jones-Bey 2013-03-29 14:13:42 PDT
Created attachment 195798 [details]
Patch

Fix layer handling for overflow hidden and shapes
Comment 3 kov's GTK+ EWS bot 2013-03-29 15:20:15 PDT
Comment on attachment 195798 [details]
Patch

Attachment 195798 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17369111
Comment 4 Bem Jones-Bey 2013-03-29 15:35:36 PDT
Comment on attachment 195798 [details]
Patch

Since the GTK build bots are completely broken right now and the GTK EWS bot's output doesn't have any error whatsoever that's related to my patch, I contend that it's failure is entirely unrelated to my patch. So I'm going to put the cq? back.
Comment 5 Julien Chaffraix 2013-04-01 14:52:13 PDT
Comment on attachment 195798 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:5850
> +            && !(renderer()->style()->shapeOutside() && renderer()->isFloating());

RenderObject::isFloating is probably cheaper to evaluate so you would be better off changing the order.

This starts to be a pattern in the code where you check renderer()->style()->shapeOutside() && renderer()->isFloating() so it should be refactored into its own helper method.

> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-overflow-hidden-expected.html:1
> +<html>

No doctype!

> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-overflow-hidden-expected.html:8
> +  background-color: red;

Let's not put any red in the expected result as it would make the test pass, even if you are saying we shouldn't see any red.

> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-overflow-hidden.html:1
> +<html>

Same comment.
Comment 6 Build Bot 2013-04-01 23:54:59 PDT
Comment on attachment 195798 [details]
Patch

Attachment 195798 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17405042
Comment 7 Bem Jones-Bey 2013-04-02 10:22:09 PDT
(In reply to comment #5)
> This starts to be a pattern in the code where you check renderer()->style()->shapeOutside() && renderer()->isFloating() so it should be refactored into its own helper method.

Doing this right involves a bunch more changes than I am confortable including in this patch right now, so I've filed Bug 113799 to do the refactoring. (I did the straightforward change, and introduced a crash, so I figure that I need to spend more time on it)
Comment 8 Bem Jones-Bey 2013-04-02 10:40:59 PDT
Created attachment 196176 [details]
Patch

Update for review comments
Comment 9 Max Vujovic 2013-04-02 10:47:11 PDT
Comment on attachment 196176 [details]
Patch

Setting cq+ for Bem.
Comment 10 WebKit Review Bot 2013-04-02 11:08:36 PDT
Comment on attachment 196176 [details]
Patch

Clearing flags on attachment: 196176

Committed r147463: <http://trac.webkit.org/changeset/147463>
Comment 11 WebKit Review Bot 2013-04-02 11:08:40 PDT
All reviewed patches have been landed.  Closing bug.