implement and test vertical alignment and relative positioning for CSS Ruby

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: dbaron, Assigned: xidorn)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

Inline layout does vertical alignment and relative positioning in a separate pass at the end of reflow.  It's not clear how well this pass interacts with the current CSS ruby code.  This needs to be tested and likely fixed.

It's also not clear on which ruby elements the 'vertical-align' property should be supported.  We should make sure the spec is solid here and then implement what it says.
Assignee

Updated

5 years ago
Depends on: 1116037
Assignee

Comment 1

5 years ago
Assignee: nobody → quanxunzhen
Attachment #8543206 - Flags: review?(dbaron)
Assignee

Comment 3

5 years ago
It seems that giving the elements correct overflow areas fix bug 1111891. Maybe this patch should be in that bug.
Attachment #8543208 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Blocks: 1055667
Comment on attachment 8543206 [details] [diff] [review]
patch 1 - Avoid calling GetLogicalBaseline before rtc being reflowed

Could you explain what problem this is fixing?

I feel like this should either (a) not be needed because we break out of the loop when we hit an RTC or (b) might break positioning of text-decorations when specified on RTC.
Flags: needinfo?(quanxunzhen)
Assignee

Comment 5

5 years ago
The current code tries to get the baseline of an RTC before it is reflowed, which is fixed by this patch.

I guess it would be better to add code to make RTCs behave more like a block.
Flags: needinfo?(quanxunzhen)
Assignee

Comment 6

5 years ago
The call path without patch 1 woule be nsRubyBaseContainerFrame::Reflow -> RelativePositionFrames -> ??? -> GetTextDecorations -> nsRubyTextContainerFrame::GetLogicalBaseline.
Comment on attachment 8543207 [details] [diff] [review]
patch 2 - Add relative positioning support for ruby annotation

>diff --git a/layout/generic/nsRubyFrame.cpp b/layout/generic/nsRubyFrame.cpp
>+
>+  for (nsFrameList::Enumerator e(mFrames); !e.AtEnd(); e.Next()) {
>+    nsIFrame* frame = e.get();
>+    if (frame->GetType() == nsGkAtoms::rubyTextContainerFrame) {
>+      // Consider only the text containers because line layout will
>+      // take care of the base containers.
>+      ConsiderChildOverflow(aDesiredSize.mOverflowAreas, frame);
>+    }
>+  }
> }

Could you explain this?

I don't think the overflow area produced here even get saved, since there is no call to FinishAndStoreOverflow.  That's fine, though, since RelativePoitionFrames will do that.  But how does this code add anything?

Also, could you add some tests showing that rel pos works on ruby?
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #5)
> The current code tries to get the baseline of an RTC before it is reflowed,
> which is fixed by this patch.
> 
> I guess it would be better to add code to make RTCs behave more like a block.

But if it needs the right baseline, won't something (like positioning of underline) be wrong if you don't have it?
Attachment #8543208 - Flags: review?(dbaron) → review+
Assignee

Comment 9

5 years ago
(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #7)
> Comment on attachment 8543207 [details] [diff] [review]
> patch 2 - Add relative positioning support for ruby annotation
> 
> >diff --git a/layout/generic/nsRubyFrame.cpp b/layout/generic/nsRubyFrame.cpp
> >+
> >+  for (nsFrameList::Enumerator e(mFrames); !e.AtEnd(); e.Next()) {
> >+    nsIFrame* frame = e.get();
> >+    if (frame->GetType() == nsGkAtoms::rubyTextContainerFrame) {
> >+      // Consider only the text containers because line layout will
> >+      // take care of the base containers.
> >+      ConsiderChildOverflow(aDesiredSize.mOverflowAreas, frame);
> >+    }
> >+  }
> > }
> 
> Could you explain this?
> 
> I don't think the overflow area produced here even get saved, since there is
> no call to FinishAndStoreOverflow.  That's fine, though, since
> RelativePoitionFrames will do that.  But how does this code add anything?

aDesiredSize.mOverflowAreas will be saved by the nsLineLayout which reflows this frame.

> Also, could you add some tests showing that rel pos works on ruby?

OK, I'll try.
Assignee

Comment 10

5 years ago
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #8)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #5)
> > The current code tries to get the baseline of an RTC before it is reflowed,
> > which is fixed by this patch.
> > 
> > I guess it would be better to add code to make RTCs behave more like a block.
> 
> But if it needs the right baseline, won't something (like positioning of
> underline) be wrong if you don't have it?

I don't think this patch makes anything worse at least, since RTCs should not be considered like inline frames here anyway, which is what the current code does. With this patch, the underline still looks fine to me, except when its children have vertical-align other than baseline, and their height varies.

Maybe I should implement an RTC-version of LazyGetLineBaselineOffset here. But I guess it worth another bug, since it is not that trivial. This patch is meant to avoid touching the assertion in GetLogicalBaseline when patch 2 lands.
Flags: needinfo?(quanxunzhen)
Assignee

Updated

5 years ago
Attachment #8543207 - Flags: review?(dbaron)
Assignee

Comment 12

5 years ago
Attachment #8543207 - Attachment is obsolete: true
Attachment #8544830 - Flags: review?(dbaron)
Assignee

Comment 13

5 years ago
Attachment #8544831 - Flags: review?(dbaron)
Assignee

Comment 14

5 years ago
Comment on attachment 8543208 [details] [diff] [review]
patch 3 - Remove fuzzy in css-ruby reftest

I failed to fix this fuzzy with the new patchset. It seems the visual overflow should have been propagated properly, but the fuzzy still exists.
Attachment #8543208 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
BTW, since I'm no longer going to introduce new member in RubyReflowState (which was the reason this struct is added in bug 1116037), should I revert the last two patches of that bug?
Comment on attachment 8543206 [details] [diff] [review]
patch 1 - Avoid calling GetLogicalBaseline before rtc being reflowed

Does vertical-align apply to elements with display:ruby-text?

If it doesn't, then this doesn't break anything, although it's hard-coding in the fact that vertical-align doesn't apply to ruby-text in a very non-obvious way.

I'd also still like to know why you're calling RelativePositionFrames before the frame has been reflowed.
Flags: needinfo?(quanxunzhen)
Attachment #8544829 - Flags: review?(dbaron) → review+
Assignee

Comment 17

5 years ago
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #16)
> Comment on attachment 8543206 [details] [diff] [review]
> patch 1 - Avoid calling GetLogicalBaseline before rtc being reflowed
> 
> Does vertical-align apply to elements with display:ruby-text?
> 
> If it doesn't, then this doesn't break anything, although it's hard-coding
> in the fact that vertical-align doesn't apply to ruby-text in a very
> non-obvious way.
> 
> I'd also still like to know why you're calling RelativePositionFrames before
> the frame has been reflowed.

It seems that with the new patches, this patch is no longer necessary.
Flags: needinfo?(quanxunzhen)
Assignee

Updated

5 years ago
Attachment #8543206 - Attachment is obsolete: true
Attachment #8543206 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
No longer blocks: 1055667
Assignee

Comment 19

5 years ago
Mostly a rebase.
Attachment #8544830 - Attachment is obsolete: true
Attachment #8544830 - Flags: review?(dbaron)
Attachment #8545468 - Flags: review?(dbaron)
Assignee

Updated

5 years ago
Attachment #8544831 - Attachment description: patch 4 - Reftests → patch 3 - Reftests
Assignee

Comment 20

5 years ago
Could you please reply comment 15?
Flags: needinfo?(dbaron)
Comment on attachment 8545468 [details] [diff] [review]
patch 2 - Add relative positioning support for ruby annotation

>+      WritingMode stateWM = mBlockReflowState->GetWritingMode();
>+      WritingMode frameWM = frame->GetWritingMode();
>+      pfd->mOffsets = mBlockReflowState->
>+        ComputedLogicalOffsets().ConvertTo(frameWM, stateWM);

Just assert that
  mBlockReflowState->GetWritingMode() == frame->GetWritingMode()
then then do
  pfd->mOffsets = mBlockReflowState->ComputedLogicalOffsets()
(since mBlockReflowState->frame is frame, so they should have the
same writing mode).

r=dbaron with that
Attachment #8545468 - Flags: review?(dbaron) → review+
Comment on attachment 8544831 [details] [diff] [review]
patch 3 - Reftests

It would be good to also add a test for relative positioning of rt, and perhaps also rbc, rb, and ruby.

r=dbaron, preferably with those added
Attachment #8544831 - Flags: review?(dbaron) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #15)
> BTW, since I'm no longer going to introduce new member in RubyReflowState
> (which was the reason this struct is added in bug 1116037), should I revert
> the last two patches of that bug?

I would be in favor of reverting the last 2 patches.  (Do you think otherwise?)
Flags: needinfo?(dbaron)
Assignee

Comment 24

5 years ago
Simply a backout command.
Attachment #8545492 - Flags: review?(dbaron)
Attachment #8545492 - Flags: review?(dbaron) → review+
Flags: needinfo?(quanxunzhen)
Assignee

Comment 28

5 years ago
I've fixed the failures on relative-positioning-2, but I don't see the failure of relative-positioning-1 in my local Mac build. I guess it is probably because of some other commit before, and has been fixed in the latest head. I have pushed another try to verify:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8949bf3543
Flags: needinfo?(quanxunzhen)
Assignee

Comment 29

5 years ago
I cannot reproduce the reftest failure on my local mac build. I'm using 10.9 while the try server runs 10.6 and 10.8, which may cause the difference.

May I simply mark that test fails-if(cocoaWidget) with a followup bug and land this patchset?
Flags: needinfo?(dbaron)
Does http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-macosx64/1420719659/mozilla-inbound_snowleopard_test-reftest-bm107-tests1-macosx-build3303.txt.gz&only_show_unexpected=1 help to figure out what's wrong?  (Could it be a sub-pixel vertical positioning difference?)

If you can't figure it out, it's probably ok to land, but different platform show subpixel positioning difference to different degrees, so it might be worth seeing if there's a sub-pixel difference.
Flags: needinfo?(dbaron)
Assignee

Updated

5 years ago
Blocks: 1055667
Assignee

Comment 31

5 years ago
It's strange that I use the same method to build the two tests, but relative-positioning-2 can always pass while relative-positioning-1 cannot.
Assignee

Updated

5 years ago
Depends on: 1120280
Assignee

Updated

5 years ago
Depends on: 1120313

Updated

3 years ago
Depends on: 1286889
You need to log in before you can comment on or make changes to this bug.