Bug 128631

Summary: [CSS Shapes] shape-outside does not properly handle different writing modes
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: Layout and RenderingAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128685    
Bug Blocks: 98664    
Attachments:
Description Flags
Do not land this patch
none
Patch
none
Patch
none
Patch
none
Patch none

Bem Jones-Bey
Reported 2014-02-11 15:45:13 PST
ShapeOutsideInfo::updateDeltasForContainingBlockLine doesn't properly convert from the writing mode and coordinate space of the containing block to and from the coordinate space of the shape. Some of this is due to the confusing names of the variables involved, some is due to the fact that the logicalLeftOffset(), logicalTopOffset() and other methods in ShapeInfo use the writing mode of the float, not of the parent. This bug tracks fixing all of these issues.
Attachments
Do not land this patch (29.33 KB, patch)
2014-02-11 15:46 PST, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
Patch (48.50 KB, patch)
2014-02-14 14:26 PST, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
Patch (48.53 KB, patch)
2014-02-18 13:16 PST, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
Patch (48.52 KB, patch)
2014-02-18 13:17 PST, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
Patch (48.61 KB, patch)
2014-02-18 15:33 PST, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Bem Jones-Bey
Comment 1 2014-02-11 15:46:13 PST
Created attachment 223909 [details] Do not land this patch Large patch for prelimary perusal. Real patch would be split into smaller pieces and include things like tests.
Bem Jones-Bey
Comment 2 2014-02-14 14:26:40 PST
Created attachment 224255 [details] Patch I tried breaking this up into multiple patches, but had trouble coming up with tests for each piece.
Bem Jones-Bey
Comment 3 2014-02-18 11:44:25 PST
Comment on attachment 224255 [details] Patch I've gotta add a couple of ASSERT_NOT_REACHED to make EFL happy.
Dave Hyatt
Comment 4 2014-02-18 11:51:14 PST
Comment on attachment 224255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224255&action=review r=me with a rename suggestion. > Source/WebCore/rendering/RenderBoxModelObject.h:121 > + virtual LayoutUnit borderWidth() const { return borderLeft() + borderRight(); } > + virtual LayoutUnit borderHeight() const { return borderTop() + borderBottom(); } I would suggest naming these horizontalBorderExtent and verticalBorderExtent. I dislike using terms like "width" since it could be confused with the actual border-width CSS name, i.e., a person new to this code would think the method was returning the pixel width of a single border. I think renaming all of the other functions would be good too, i.e., marginWidth = horizontalMarginExtent, marginHeight = verticalMarginExtent, etc. borderAndPaddingWidth could be horizontalBorderAndPaddingExtent, and then borderAndPaddingHeight could be verticalBorderAndPaddingExtent.
Bem Jones-Bey
Comment 5 2014-02-18 13:16:41 PST
Created attachment 224536 [details] Patch Will do renames in a followup patch
Bem Jones-Bey
Comment 6 2014-02-18 13:17:58 PST
Created attachment 224537 [details] Patch forgot to update reviewer
Bem Jones-Bey
Comment 7 2014-02-18 15:33:39 PST
Created attachment 224560 [details] Patch My ability to read error messages is apparently a problem. I need a return statement to shut that EFL compiler up!
WebKit Commit Bot
Comment 8 2014-02-19 07:14:46 PST
Comment on attachment 224560 [details] Patch Clearing flags on attachment: 224560 Committed r164363: <http://trac.webkit.org/changeset/164363>
WebKit Commit Bot
Comment 9 2014-02-19 07:14:49 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.