Closed Bug 1132008 Opened 5 years ago Closed 5 years ago

Implement correct block-axis sizing of ruby text container

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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
Attachment #8563238 - Flags: review?(dbaron)
Attachment #8563240 - Flags: review?(dbaron)
Attachment #8563241 - Flags: review?(dbaron)
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+
Blocks: 1120280
You need to log in before you can comment on or make changes to this bug.