Closed Bug 169139 Opened 22 years ago Closed 18 years ago

Direction of inline replaced elements affects their order within the line

Categories

(Core :: Layout: Text and Fonts, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: bugzilla, Assigned: uriber)

References

(
URL
)

Details

Attachments

(4 files, 2 obsolete files)

Expanded testcase
1.98 KB, text/html
Details
some more testcases
1.30 KB, text/html
Details
Testcase for first-letter
383 bytes, text/html
Details
patch v3
8.28 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1) Gecko/20020826
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1) Gecko/20020826

The following HTML code:
<html>
<body dir="rtl">

<form>
	<select name="1" dir="ltr">
		<option>first</option>
	</select>
	<select name="2" dir="ltr">
		<option>second</option>
	</select>

</form>

</body>
</html>

The "second" appers to be the first from the right. If I remove the dir="ltr"
inside the select it works right, first is the first from the right side.

I belive this is wrong - <select dir="ltr"> should effect the items inside the
select box and not it's parent.

Reproducible: Always

Steps to Reproduce:
Use this HTML code:

<html>
<body dir="rtl">

<form>
	<select name="1" dir="ltr">
		<option>first</option>
	</select>
	<select name="2" dir="ltr">
		<option>second</option>
	</select>

</form>

</body>
</html>
Actual Results:  
Second is the first select from the right side.

Expected Results:  
First should be the first.
Summary: Select box direction effects parent → Select box direction affects parent
I don't agree with your logic: since the <select>s are left-to-right inline
elements within a right-to-left block, they should be displayed as

                                                              first   second

just the same as the words "first second" would be.

On the other hand, both IE 5.5 and Konqueror display your testcase as

                                                              second   first

so I am not 100% sure.
I tend to agree, then, that this is a bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
=> All/All
OS: Windows XP → All
Hardware: PC → All
Attached patch maybe like this? (obsolete) — — Splinter Review 

        Attachment #216333 -
        Flags: review?(smontagu)
Summary: Select box direction affects parent → Direction of inline replaced elements affects their order within the line

    
  

        Attachment #216333 -
        Attachment is obsolete: true

        Attachment #216333 -
        Flags: review?(smontagu)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Only embed/override frames of type inlineFrame (and positionedInlineFrame). This fixes the issue for images, as well as various form controls.
Assignee: mozilla → uriber
Status: NEW → ASSIGNED

        Attachment #231734 -
        Flags: review?(smontagu)

    
    

    
  

      
      
      
      

        Attached file
          
        some more testcases
        — 
      

    


  
The ordering of images and various form controls should not be affected by the "dir" attribute on these elements.
The ordering of spans (and inlined divs) should be affected.

    
    

    
  
Comment on attachment 231734 [details] [diff] [review]
patch v2

Maybe you thought I was joking when I said "don't ask me to review this". I really think you should ask someone who understands frameTypes a whole lot better than I do.

        Attachment #231734 -
        Flags: review?(smontagu)

    
  

        Attachment #231734 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 231734 [details] [diff] [review]
patch v2

I assume we never see a lineFrame come through here?  Or don't need to do anything interesting with it?

What about first-letter frames?

In general, this seems like something better handled by a IsFrameOfType() bit than a frameType check; we're generally trying to move away from lists of frametypes.

    
    

    
  


  
This testcase shows that unicode-bidi styling is not applied to first-letter. This is true on current trunk, with the proposed patch, or with a modified version of it which adds letterFrame to the frame types for which embedding frames are created.
The reason is that nsStyleTextReset.mUnicodeBidi is not being set for the first-letter frame.
So this is a separate bug.

    
    

    
  
(In reply to comment #9)
> (From update of attachment 231734 [details] [diff] [review] [edit])
> I assume we never see a lineFrame come through here?  Or don't need to do
> anything interesting with it?

True. We never see a lineFrame here.

> What about first-letter frames?

In principal they should be handled as well, so I'll add "|| frameType == nsLayoutAtoms::letterFrame" to the condition if we decide to go this way.
In practice this won't change anything - see comment #10. Notice also that first-letter in bidi is severely broken anyway - see bug 198928.

> In general, this seems like something better handled by a IsFrameOfType() bit
> than a frameType check; we're generally trying to move away from lists of
> frametypes.

I can try adding this bit if you think it's generally useful. If you'd like me to do this, I'd appreciate a suggestion for a name for the bit, and confirmation that inlineFrame, positionedInlineFrame, and letterFrame are the only types of frames for which the bit should be set.

    
    

    
  
> I can try adding this bit if you think it's generally useful.

Even if it's only useful in this one single place but makes this code less fragile, we should do it, I think.

> I'd appreciate a suggestion for a name for the bit

eParticipatesInBidiReordering?  I don't know what the code you're changing here actually does, so...

> and confirmation that inlineFrame, positionedInlineFrame, and letterFrame are 
> the only types of frames for which the bit should be set.

I _think_ that's correct, but my bidi knowledge is pretty slim, to be truthful.  I have no idea what bidi _should_ do in general.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3
        — 
          — Splinter Review
      

    


  
Define a "eBidiInlineContainer" type, set it for ns[Positioned]InlineFrame and nsFirstLetterFrame, and test for it in various places in nsBidiPresUtils which previously tested for a list of frame types.

        Attachment #231734 -
        Attachment is obsolete: true

        Attachment #231996 -
        Flags: review?(bzbarsky)

        Attachment #231734 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 231996 [details] [diff] [review]
patch v3

>Index: layout/base/nsBidiPresUtils.cpp
>+    || !(aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)
>          || nsLayoutAtoms::blockFrame == frameType);

As a followup, why blockFrame but not areaFrame or fieldsetFrame or other block-like things?

r+sr=bzbarsky.

        Attachment #231996 -
        Flags: superreview+

        Attachment #231996 -
        Flags: review?(bzbarsky)

        Attachment #231996 -
        Flags: review+

    
    

    
  
(In reply to comment #14)
> (From update of attachment 231996 [details] [diff] [review] [edit])
> >Index: layout/base/nsBidiPresUtils.cpp
> >+    || !(aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)
> >          || nsLayoutAtoms::blockFrame == frameType);
> 
> As a followup, why blockFrame but not areaFrame or fieldsetFrame or other
> block-like things?
> 
> r+sr=bzbarsky.
> 

Actually, I would think the block-like things are covered by the |aFrame->GetStyleDisplay()->IsBlockLevel()| test (one line above), so I'm not even sure why the test for nsLayoutAtoms::blockFrame is there.
Can you think of cases where IsBlockLevel() would return FALSE for block-type frames, or TRUE for non-block-type frames?

I'll check in the patch as-is for now, and, as you suggested, I'll deal with this issue as a followup.

    
    

    
  
Checked in:
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.327; previous revision: 3.326
done
Checking in layout/generic/nsInlineFrame.h;
/cvsroot/mozilla/layout/generic/nsInlineFrame.h,v  <--  nsInlineFrame.h
new revision: 1.54; previous revision: 1.53
done
Checking in layout/generic/nsInlineFrame.cpp;
/cvsroot/mozilla/layout/generic/nsInlineFrame.cpp,v  <--  nsInlineFrame.cpp
new revision: 3.260; previous revision: 3.259
done
Checking in layout/generic/nsFirstLetterFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFirstLetterFrame.cpp,v  <--  nsFirstLetterFrame.cpp
new revision: 1.63; previous revision: 1.62
done
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.84; previous revision: 1.83
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha

    
    

    
  
> Can you think of cases where IsBlockLevel() would return FALSE for block-type
> frames, or TRUE for non-block-type frames?

What do you mean by "block-type frames"?

Basically, the display of the frame and the concrete class implementing it are somewhat independent.  For example, of you have a replaced inline (including fieldsets), the concrete frame class could be nsBlockFrame or some subclass thereof (nsAreaFrame is common).

And of course once we implement inline-block....

Note that there are really two separate concepts of "block level" in CSS.  Think of them as "looks like a block from the inside" and "looks like a block from the outside".  Which one are you testing for?

    
    

    
  
(In reply to comment #17)
> > Can you think of cases where IsBlockLevel() would return FALSE for block-type
> > frames, or TRUE for non-block-type frames?
> 
> What do you mean by "block-type frames"?

I meant whatever you meant by "[frames of type] blockFrame[,] areaFrame[,] fieldsetFrame or other block-like things" :-)

> Note that there are really two separate concepts of "block level" in CSS. 
> Think of them as "looks like a block from the inside" and "looks like a block
> from the outside".  Which one are you testing for?
> 

I'm pretty sure I'm[*] testing for "looks like block from the inside". Anything which "looks like block from the inside" should be considered an atomic element for the purpose of bidi resolution of its containing block, regardless of its actual content. Hence it is considered a "leaf".

Which of these two concepts is IsBlockLevel() testing for? I actually suspect it's the other one ("from the outside").

Thinking about this some more, I don't understand why the test for |nsLayoutAtoms::blockFrame == frameType| is there at all. It implies that the content of block frames (of some type) should be allowed to participate in the bidi ordering of those frames' containing block, which seems wrong to me.

Simon, can you perhaps provide some insight into this? 

[*](Note that it's not really "me" who is testing - I didn't write this condition originally, I just refactored it into a method at some point).

    
    

    
  
> Which of these two concepts is IsBlockLevel() testing for?

"Looks like a block from the outside".

Fwiw, I did CVS archeology, and this is part of the original BiDi landing, with no explanation as to why.  :(

    
    

    
  
(In reply to comment #19)

> Fwiw, I did CVS archeology, and this is part of the original BiDi landing, with
> no explanation as to why.  :(
> 

I did that myself earlier. Sorry - I could have saved you the time.

Testing shows that inlined fieldsets do behave correctly (or at least, according to what I deem to be correct behavior) with the current code, thanks to the fact that nsLayoutAtoms::fieldSetFrame is *not* included in that condition alongside nsLayoutAtoms::blockFrame.

Can you think of an example where a frame would report its type as nsLayoutAtoms::blockFrame, but would look like an inline "from the outside" (i.e., IsBlockLevel() returns FALSE for it)?

    
    

    
  
Not offhand.  I think all such would return areaFrame or some such as the type.
Depends on: 351393

    
    

    
  
(In reply to comment #20)
> Can you think of an example where a frame would report its type as
> nsLayoutAtoms::blockFrame, but would look like an inline "from the outside"
> (i.e., IsBlockLevel() returns FALSE for it)?

<div style="display:-moz-inline-block"></div>

GetStyleDisplay()->IsBlockLevel() is PR_FALSE
GetType() is nsLayoutAtoms::blockFrame

Depends on: 364839

    
  
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: