Bug 110349 - [css exclusions] overflow:hidden undoes shape-outside offsets
Summary: [css exclusions] overflow:hidden undoes shape-outside offsets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL: http://codepen.io/btravis/pen/HuecB
Keywords:
Depends on:
Blocks: 98664
  Show dependency treegraph
 
Reported: 2013-02-20 07:32 PST by Bear Travis
Modified: 2013-04-02 11:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2013-03-29 14:13 PDT, Bem Jones-Bey
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff
Patch (5.64 KB, patch)
2013-04-02 10:40 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.