Implement correct block-axis sizing of ruby text container

RESOLVED FIXED in Firefox 38

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(4 attachments)

As it has not been completely clear what should happen, and/or how should block-axis sizing be implemented for ruby base container, and the current behavior of rbcs are fine in most cases, it is decided to implement the sizing for ruby text containers first.
Assignee: nobody → quanxunzhen
Created attachment 8563238 [details] [diff] [review]
patch 1 - Calculate bsize of rtc
Attachment #8563238 - Flags: review?(dbaron)
Created attachment 8563239 [details] [diff] [review]
patch 2 - Sync bounds of <rt>s in line layout
Attachment #8563239 - Flags: review?(dbaron)
Created attachment 8563240 [details] [diff] [review]
patch 3 - Remove unused code
Attachment #8563240 - Flags: review?(dbaron)
Created attachment 8563241 [details] [diff] [review]
patch 4 - Update reftests
Attachment #8563241 - Flags: review?(dbaron)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39a7945c7b69
Blocks: 1112474
Comment on attachment 8563238 [details] [diff] [review]
patch 1 - Calculate bsize of rtc

>+    nsCSSOffsetState offsetState(child, aReflowState.rendContext, 0);
>+    LogicalMargin margin = offsetState.ComputedLogicalMargin()
>+      .ConvertTo(lineWM, offsetState.GetWritingMode());

Is there a reason you can't use child->GetUsedMargin() instead of
constructing a nsCSSOffsetState (which is somewhat heavyweight)?

Or maybe even add and use nsIFrame::GetMarginRect() (but note in
a comment that it doesn't make sense for elements whose margins collapse)?

>+    nscoord bstart = rect.BStart(lineWM) - margin.BStart(lineWM);
...
>+    nscoord bend = rect.BEnd(lineWM) + margin.BEnd(lineWM);

Please capitalize as bStart and bEnd.

>+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
>+        pos.B(lineWM) += deltaBCoord;
>+        child->SetPosition(lineWM, pos, containerWidth);

This doesn't do the right thing for sticky positioning.  You should
use nsIFrame::MovePositionBy() instead.

r=dbaron with that
Attachment #8563238 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #6)
> Comment on attachment 8563238 [details] [diff] [review]
> patch 1 - Calculate bsize of rtc
> 
> >+    nsCSSOffsetState offsetState(child, aReflowState.rendContext, 0);
> >+    LogicalMargin margin = offsetState.ComputedLogicalMargin()
> >+      .ConvertTo(lineWM, offsetState.GetWritingMode());
> 
> Is there a reason you can't use child->GetUsedMargin() instead of
> constructing a nsCSSOffsetState (which is somewhat heavyweight)?

I guess I can use this.

> Or maybe even add and use nsIFrame::GetMarginRect() (but note in
> a comment that it doesn't make sense for elements whose margins collapse)?
> 
> >+    nscoord bstart = rect.BStart(lineWM) - margin.BStart(lineWM);
> ...
> >+    nscoord bend = rect.BEnd(lineWM) + margin.BEnd(lineWM);
> 
> Please capitalize as bStart and bEnd.

I'd prefer blockStart and blockEnd if we have to use this style.

> >+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
> >+        pos.B(lineWM) += deltaBCoord;
> >+        child->SetPosition(lineWM, pos, containerWidth);
> 
> This doesn't do the right thing for sticky positioning.  You should
> use nsIFrame::MovePositionBy() instead.

No, I can't. I tried using nsIFrame::MovePositionBy, but it produces the incorrect result for relative positioning. The comment above that method also says it should be used after relative positioning.
(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #6)
> >+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
> >+        pos.B(lineWM) += deltaBCoord;
> >+        child->SetPosition(lineWM, pos, containerWidth);
> 
> This doesn't do the right thing for sticky positioning.  You should
> use nsIFrame::MovePositionBy() instead.

Actually, I guess relative positioning hasn't happened yet.  So, instead, you should keep it this way, but add a comment explaining that relative position has not happened yet, so you don't want MovePositionBy.
Comment on attachment 8563239 [details] [diff] [review]
patch 2 - Sync bounds of <rt>s in line layout

>+ * For containers, their are not part of the line in their levels,
>+ * which means their bounds are not set properly before.

"For containers, their" -> "Containers"


It's confusing to call the rect of the rtc "bounds" and the rect of
the rt "rect".  Could you call them rtcRect and rtRect, or
rtcBounds and rtBounds?

r=dbaron with that
Attachment #8563239 - Flags: review?(dbaron) → review+
Attachment #8563240 - Flags: review?(dbaron) → review+
Attachment #8563241 - Flags: review?(dbaron) → review+
try push in comment 5

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad44003f7be
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cfa37df70
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b496503037
https://hg.mozilla.org/integration/mozilla-inbound/rev/426d022857ff
https://hg.mozilla.org/mozilla-central/rev/2ad44003f7be
https://hg.mozilla.org/mozilla-central/rev/2d9cfa37df70
https://hg.mozilla.org/mozilla-central/rev/a6b496503037
https://hg.mozilla.org/mozilla-central/rev/426d022857ff
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1120280
You need to log in before you can comment on or make changes to this bug.