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
1.22 KB, image/gif
Details
screenshot with nbsp as anchor content
1.21 KB, image/gif
Details
proposed fix
4.03 KB, text/html
Details
proposed patch (again, previous one formatted wrong)
4.03 KB, patch
Details | Diff | Splinter Review
strict-dtd tastcase with an empty list item
202 bytes, 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 —
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...
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 —
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
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: