Closed Bug 1141928 Opened 9 years ago Closed 9 years ago

Ruby annotation is not placed correctly for RTL content

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

patch 3 - reftest
2.23 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
part 2 (alternative) - Correct the position of ruby text frames
5.07 KB, patch
roc
: review+
xidorn
: feedback+
Details | Diff | Splinter Review
patch 0 - change line relative dir mapping code
2.90 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
patch 1 - correct ruby text containers
9.33 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review 
Testcase:
data:text/html,<body dir="rtl"><ruby>base<rt>text</ruby>
Please note that the testcase in comment 0 could crash debug build because of violation of an assertion.
Blocks: 1141931
Assignee: nobody → quanxunzhen

        Attachment #8575939 -
        Flags: review?(jfkthame)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 2 - correct ruby text (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8575940 -
        Flags: review?(jfkthame)

    
    

    
  


  

        Attachment #8575942 -
        Flags: review?(jfkthame)

    
    

    
  
Actually, these patches are not enough for fixing ruby in RTL content. I plan to fix the left parts in bug 1141931.

    
    

    
  
Comment on attachment 8575942 [details] [diff] [review]
patch 3 - reftest

Review of attachment 8575942 [details] [diff] [review]:
-----------------------------------------------------------------

Look OK. I notice that without color:transparent, the "text" annotation is actually misplaced (doing Select All also reveals this, with the highlight being offset from the expected rect). I assume this is part of what you're expecting to handle in bug 1141931, and will add more tests at that time.

        Attachment #8575942 -
        Flags: review?(jfkthame) → review+

    
    

    
  
Comment on attachment 8575939 [details] [diff] [review]
patch 1 - correct ruby text containers

Review of attachment 8575939 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with a minor suggested change (if you agree that it makes sense).

::: layout/generic/nsLineLayout.cpp
@@ +2982,5 @@
> +      // It is necessary to set the rect again because the container
> +      // width was unknown, and zero was used instead when we reflow
> +      // them. The corresponding base containers were repositioned in
> +      // VerticalAlignFrames and PlaceTopBottomFrames.
> +      annotation->mFrame->SetRect(lineWM, annotation->mBounds, aContainerWidth);

If I'm understanding things, this should only ever change the annotation frame's position, not size; so how about adding

  annotation->mFrame->GetLogicalSize(lineWM) ==
    annotation->mBounds.Size(lineWM)

to the assertion, and using SetPosition rather than SetRect.

(Or are there cases where the size might be changing as well?)

        Attachment #8575939 -
        Flags: review?(jfkthame) → review+

    
    

    
  
Comment on attachment 8575940 [details] [diff] [review]
patch 2 - correct ruby text

Review of attachment 8575940 [details] [diff] [review]:
-----------------------------------------------------------------

Holding off on review here for now; the patch appears to work, but the end result feels a bit hacky to me. I've tried putting together an alternative approach that IMO results in cleaner code overall, which I'll post as a separate patch for you to consider, then let's decide how to go forward.

    
    

    
  


  
Here's my alternative suggestion for patch 2; seems to work in my local testing so far. See what you think...

        Attachment #8576828 -
        Flags: review?(quanxunzhen)

    
    

    
  
Comment on attachment 8576828 [details] [diff] [review]
part 2 (alternative) - Correct the position of ruby text frames

Review of attachment 8576828 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I agree your patch is better than mine. But I wonder whether I can review this. This is not a trivial patch, and I'm not a peer. If I can, I'll grant it, though.

        Attachment #8576828 -
        Flags: review?(quanxunzhen) → feedback+

    
  

        Attachment #8575940 -
        Attachment is obsolete: true

        Attachment #8575940 -
        Flags: review?(jfkthame)

    
  

        Attachment #8576828 -
        Flags: review?(roc)

    
    

    
  


  

        Attachment #8577081 -
        Flags: review?(jfkthame)

    
    

    
  


  
I just realized that I did the wrong change. We cannot get rid of line relative dir mapping code there, because logical side and line relative dir are different.

        Attachment #8575939 -
        Attachment is obsolete: true

        Attachment #8577082 -
        Flags: review?(jfkthame)

    
    

    
  


  
The previous one causes bustage on non-Win platforms

        Attachment #8577081 -
        Attachment is obsolete: true

        Attachment #8577081 -
        Flags: review?(jfkthame)

        Attachment #8577090 -
        Flags: review?(jfkthame)

    
    

    
  


  

        Attachment #8577082 -
        Attachment is obsolete: true

        Attachment #8577082 -
        Flags: review?(jfkthame)

        Attachment #8577091 -
        Flags: review?(jfkthame)

        Attachment #8577090 -
        Flags: review?(jfkthame) → review+

        Attachment #8577091 -
        Flags: review?(jfkthame) → review+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: