Closed Bug 50480 Opened 24 years ago Closed 24 years ago

[LIST][MARGIN-C][INLINE-V]list-item marker of link list overlaps

Categories

(Core :: DOM: Editor, defect, P1)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: kazhik, Assigned: buster)

References

Details

(Whiteboard: [fix in hand] [rtm++])

Attachments

(5 files)

screenshot
(deleted), image/gif
Details
screenshot with nbsp as anchor content
(deleted), image/gif
Details
proposed fix
(deleted), text/html
Details
proposed patch (again, previous one formatted wrong)
(deleted), patch
Details | Diff | Splinter Review
strict-dtd tastcase with an empty list item
(deleted), text/html
Details 
list-item marker of link list overlaps

(1) Open composer window.
(2) Enter a text(ex. "www.mozilla.org") and select it.
(3) Select [Format]-[List]-[Numbered] and [Insert]-[Link].
(4) Move the mouse pointer to the end of the line and hit Enter key.
(5) List-item marker "2" overlaps "1".

Observed on Win98 2000082608.
it seems that you must be within the link to have this happen, using bodytext 
results in the correct behavior. In any event, this is a layout issue, passing 
this one over to buster
Assignee: beppe → buster
Keywords: correctness, nsbeta3
Sending this to Joe: probably a dup of a know or fixed bug.
Assignee: buster → jfrancis
Status: UNCONFIRMED → NEW
Ever confirmed: true
this should go to layout, the html is correct:
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  <title>50480</title>
</head>
<body>
<ol>
  <li><a href="http://www.mozilla.org">http://www.mozilla.org</a></li>
  <li><a href="http://www.mozilla.org"></a></li>
</ol>
</body>
</html>

------------
If I save the file, and display it in the browser, it renders the same as in
composer -- the numbers overlap. I have to insert a character or nbsp to get the
correct rendering.
Attached image screenshot (deleted) — 

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot with nbsp as anchor content (deleted)
        — 
      

    


  

    
    

    
  
handing over to buster

Assignee: jfrancis → buster

    
    

    
  
Looks like the empty <A></A> isn't triggering any line height in this case.  
I tried some simple style sheet work-arounds, like explicitly setting the line 
height of an empty <A>, and that didn't work (but should!)
PDT: the result of this bug is that users have no obvious way to type into the 
second list item, since the first visually overlays it.  The end user can, in 
fact, click on the first list item, press "end" to move the caret to the end of 
the line, then press "right arrow" to advance the caret to the empty <A>, and 
start typing.  Once the first character is inserted, the display is correct.

This is an unlikely edge case for the browser, but a common case for the editor.  
Please make a priority appraisal so I know if I should look at this for beta3, 
fcs, or future.

Risk to fix:  currently unknown.  Likely to be low, since I think I'll just 
check for lines which contain only empty inline elements, and give them the line 
height specified by the current font.  Ian, David:  does that sound like a 
reasonable approach?

Severity: normal → major
Status: NEW → ASSIGNED
OS: Windows 98 → All
Hardware: PC → All

    
    

    
  
I think this is a dup, and I may have commented on the other bug when trying to 
fix it during my margin work.

I don't think a line-layout fix will fix this.  Quirks mode line-layout should 
already handle LI, DD, and DT elements as a special case so that the first line 
of LI elements and the last line of all 3 will always have the correct font size.  
(And this should happen even if the LI element is completely empty, so the 
suggested fix is wrong.)  (In strict mode, this happens for *everything*, not 
just those elements.)  My memory from my previous investigation of this bug is 
that there is some code somewhere that is determining that the paragraph is empty 
and should therefore be ignored, but I couldn't find it.  I attempted a fix for 
it by adding a check for a bullet to one block emptiness check, and that didn't 
work.  But I'm not sure my memory is correct, so it might be worth rechecking 
this...

#define NOISY_VERTICAL_ALIGN should tell you pretty quickly what line-layout is 
doing...


    
    

    
  
Duplicate of bug 34404, I think.


    
    

    
  
... and bug 28845 and bug 41777.


    
    

    
  
We really should change quirks-mode line-layout to do a legitimate bullet check 
rather than check for the LI element, anyway.  I'm not so sure where this bug 
lies anymore... I've made different comments on different bugs.


    
    

    
  
*** Bug 28845 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 41777 has been marked as a duplicate of this bug. ***

    
    

    
  
When I said "should be ignored" I meant "should have margins collapsed through 
it".


    
    

    
  
*** Bug 34404 has been marked as a duplicate of this bug. ***

    
  
Summary: list-item marker of link list overlaps → [LIST][MARGIN-C][INLINE-V]list-item marker of link list overlaps

    
    

    
  
Marking nsbeta3+. This is important to composer.
Whiteboard: [nsbeta3+]

    
    

    
  
buster / karnaze - should this be a higher priority if Composer really needs
this?  If this has missed nsbeta3, you may want to nominate for rtm.

Currently, per PDT: P3-P5 priority bugs changed should be changed from nsbeta3+
to nsbeta3- since we have more important work to do for Seamonkey. Since you
have a recent comment, I won't do this from under you.  I'll come back to this
bug in a day or two.


    
    

    
  
I have a fix for this.  It is very simple, very low risk.

This bug was marked beta3+. This fixes a serious usability problem in editor.
Look at the tree of duplicates to realize how common this is.  But I don't think
it's important enough to warrent checking into the branch for beta3.  However, I
do think it's important enough to get in for RTM, so I'm nominating to PDT for
RTM++.

David has pointed out there there are more "correct" possible fixes, but this
isn't the appropriate time to explore those options.

Keywords: nsbeta3 → rtm
Whiteboard: [nsbeta3+] → [fix in hand] [rtm+]

    
    

    
  

      
      
      
      

        Attached file
          
        proposed fix (deleted)
        — 
      

    


  

    
    

    
  
upped to P1 due to serious usability issue in editor.
karnaze, can you please review?
waterson, can you super-review?

Priority: P3 → P1

    
    

    
  
I vote for this bug since it seriously impedes my ability to use the editor to 
edit my status report.

    
    

    
  
I think the real fix would be much easier if we didn't create frames for 
ignorable whitespace, since we could then drop the else part (see "XXX issues") 
entirely.  (We'd have to make sure we created the bullet!)


    
    

    
  
*** Bug 41777 has been marked as a duplicate of this bug. ***

    
    

    
  


  

    
    

    
  
r=karnaze.

    
    

    
  
Not exactly sure what "foundLI" is supposed to mean, but if it means the naive 
thing, we could "find an LI" in the "else if", right?

2313           // (2) above, if the first line of LI
2314           if (isFirstLine && blockTagAtom.get() == nsHTMLAtoms::li) {
2315             applyMinLH = PR_TRUE;
+                foundLI = PR_TRUE;
2316           }
2317           // (3) above, if the last line of LI, DT, or DD
2318           if (!applyMinLH && isLastLine &&
2319                 ((blockTagAtom.get() == nsHTMLAtoms::li) ||
2320                  (blockTagAtom.get() == nsHTMLAtoms::dt) ||
2321                  (blockTagAtom.get() == nsHTMLAtoms::dd))) {
2322             applyMinLH = PR_TRUE;
                 // XXX if blockTagAtom.get() == nsHTMLAtoms::li, should
                 // we set foundLI?
2323           }

Also, if this is a short-term hack, should we add some comments referring back 
to this bug and note that this'll eventually need to be fixed mo' betta? (And 
maybe file a bug to do so...)

    
    

    
  
waterson:
1) "foundLI" means "I found an LI in a place I care about for this hack"
2) We don't care about the else clause in this case.
3) Sloppy of me not to add a comment that includes the bug number.  Done.
4) I think we should open a new bug on a more correct approach.


    
    

    
  
Ok, sounds good. a=waterson


    
    

    
  
*** Bug 54533 has been marked as a duplicate of this bug. ***

    
    

    
  
PDT marking [rtm++]

Whiteboard: [fix in hand] [rtm+] → [fix in hand] [rtm++]

    
    

    
  
I have filed bug 54979 for the Right Way fix.

    
    

    
  
fix checked into branch

Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
What about the trunk?

    
    

    
  
fix checked into tip


    
    

    
  
Verified with Win32 build 2000100420 on Win98.

Status: RESOLVED → VERIFIED

    
    

    
  
I'll attach a testcase in a mo...

bug#41777, which was marked as a DUP of this bug still happens with a strict
DTD. Is this a problem?

(I don't know if it is valid to have an empty LI tag, but the w3c validator says
this test case is OK, but the LI numbers are still overwriting each other on
mozilla:20010528.)

    
    

    
  


  

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: