Bug 69957 - display:inline-block elements don't correctly handle orthogonal writing-modes
Summary: display:inline-block elements don't correctly handle orthogonal writing-modes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-12 13:25 PDT by Ojan Vafai
Modified: 2011-10-17 18:32 PDT (History)
2 users (show)

See Also:


Attachments
test case (666 bytes, text/html)
2011-10-12 13:25 PDT, Ojan Vafai
no flags Details
Patch (6.42 KB, patch)
2011-10-12 18:34 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (6.59 KB, patch)
2011-10-14 15:56 PDT, Ojan Vafai
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-10-12 13:25:25 PDT
Created attachment 110736 [details]
test case

Summary: border-right doesn't correctly take vertical writing mode into account

See test case.
Comment 1 Ojan Vafai 2011-10-12 18:30:00 PDT
This actually has nothing to do with borders.
Comment 2 Ojan Vafai 2011-10-12 18:34:33 PDT
Created attachment 110788 [details]
Patch
Comment 3 Ojan Vafai 2011-10-12 18:35:41 PDT
Here's a first attempt. I'm a bit dubious this is the right fix given the call to computeLogicalHeight. It fixes the bug though.
Comment 4 Dave Hyatt 2011-10-13 14:23:16 PDT
Comment on attachment 110788 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:5247
> +            toRenderBox(child)->computeLogicalHeight();

Yeah this is not quite right. It will use a height from a previous layout for the auto case, and that's pretty weird. I think you need to set the intrinsic height to just be borders + padding, then do computeLogicalHeight, and then use that result. However in addition, this computation needs to not have side effects, i.e., you need to set the height back to what it was when you're finished.

Note that I'm approaching the point with region refactoring that I'm probably going to give you a way to compute the height/width without updating the box's member variables, since this hack of "compute it but then set it back" is starting to become really widespread.

So anyway to sum up:

(1) Cache the old height
(2) Set the new height to be borders and padding in the logical height direction.
(3) Compute the logical height
(4) Cache that as your preferred logical width.
(5) Set it back to the old height.
Comment 5 Ojan Vafai 2011-10-14 15:56:40 PDT
Created attachment 111101 [details]
Patch
Comment 6 Dave Hyatt 2011-10-17 12:15:46 PDT
Comment on attachment 111101 [details]
Patch

r=me
Comment 7 Ojan Vafai 2011-10-17 14:45:52 PDT
Committed r97653: <http://trac.webkit.org/changeset/97653>
Comment 8 Ojan Vafai 2011-10-17 15:52:46 PDT
Reverted r97653 for reason:

Caused a number of Chromium failures.

Committed r97661: <http://trac.webkit.org/changeset/97661>
Comment 9 Ojan Vafai 2011-10-17 18:32:14 PDT
Committed r97693: <http://trac.webkit.org/changeset/97693>