Closed Bug 1055658 Opened 10 years ago Closed 10 years ago

implement and test vertical alignment and relative positioning for CSS Ruby

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: dbaron, Assigned: xidorn)

References

Details

Attachments

(4 files, 5 obsolete files)

patch 3 - Reftests
5.26 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
patch 1 - Move some code to an independent method for reuse, r=dbaron
3.74 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
patch 2 - Add relative positioning support for ruby annotation
5.48 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
patch 4 - Revert RubyReflowState changes.
19.15 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
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.
Depends on: 1116037
Assignee: nobody → quanxunzhen

        Attachment #8543206 -
        Flags: review?(dbaron)

    
    

    
  


  
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)

    
  
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)

    
    

    
  
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)

    
    

    
  
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+

    
    

    
  
(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.

    
    

    
  
(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)

    
  

        Attachment #8543207 -
        Flags: review?(dbaron)

    
    

    
  


  

        Attachment #8543207 -
        Attachment is obsolete: true

        Attachment #8544830 -
        Flags: review?(dbaron)

    
    

    
  


  

        Attachment #8544831 -
        Flags: review?(dbaron)

    
    

    
  
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

    
    

    
  
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+

    
    

    
  
(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)

    
  

        Attachment #8543206 -
        Attachment is obsolete: true

        Attachment #8543206 -
        Flags: review?(dbaron)

    
  
No longer blocks: 1055667

    
    

    
  


  
Mostly a rebase.

        Attachment #8544830 -
        Attachment is obsolete: true

        Attachment #8544830 -
        Flags: review?(dbaron)

        Attachment #8545468 -
        Flags: review?(dbaron)

    
  

        Attachment #8544831 -
        Attachment description: patch 4 - Reftests → patch 3 - Reftests

    
    

    
  
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)

    
    

    
  


  
Simply a backout command.

        Attachment #8545492 -
        Flags: review?(dbaron)

        Attachment #8545492 -
        Flags: review?(dbaron) → review+
Flags: needinfo?(quanxunzhen)

    
    

    
  
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)

    
    

    
  
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)

    
  
Blocks: 1055667

    
    

    
  
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.

    
  
Depends on: 1120280

    
  
Depends on: 1120313

    
  
Depends on: 1286889

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: