Closed
Bug 96455
Opened 23 years ago
Closed 23 years ago
hr has duplicate lineboxes in frame tree
Categories
(Core :: DOM: Editor, defect, P3)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
mozilla0.9.7
People
(Reporter: TucsonTester2, Assigned: kinmoz)
References
Details
(Whiteboard: [EDITORBASE] fixed on trunk)
Attachments
(5 files, 3 obsolete files)
From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0b; Windows NT 5.1)
BuildID: 20010726
Horizontal Line indention and outdention gives you a line that is not
selectable. Also by choosing add number list it creats an additional line with
no number,
Reproducible: Always
Steps to Reproduce:
1. Open new composer page
2. Click on insert and choose Horizontal Line
3. Click on indent
4. Click on horizontal Line and then click on outdent
5. The line can no longer be selected
6. Click on add number list
Actual Results: There was two horizontal lines. One of them could not be
deleted and the other was in numberlist format with no number in front of it
Expected Results: Allowed the Horizontal line to be selected and deleted
To choose outdent the Horizontal line must be selected again
|
||
Comment 1•23 years ago
|
||
I see this in my Mac debug build from today; reassign to kin
I see this on my console:
frame: Inline(hr)(0) (0x1a0bd580) style: 0x1a0bd550 :after {}
Wrong parent style context style: 0x1a0bd428 {}
should be using: style: 0x1bde02e4 {}
Assignee: brade → kin
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All
Changing summary from "Horizontal Line indention and outdention gives you a line
that is not selectable" to "hr has duplicate lineboxes in frame tree"
Unless I'm misreading the frame dump in viewer, it looks like layout generates 2
lines for each HR in the content tree, which causes problems in the editor
whenever we remove and re-insert the hr into the content tree. (See JS Test Case
for an example)
Turns out the Minimal Test Case I attatched above, isn't so minimal. You can see
the duplication in the frame tree with a simple doc:
<html><body><hr></body></html>
attinasi/waterson, do either of you have a bug like this on your plate already?
I'll attatch another test case that allows you to insert and remove the same hr
in the content tree. If you do that a couple of times, you'll see a lot of
duplication in the frame tree ... as if frames aren't getting destroyed like
they should be, even when their associated content is removed from the tree.
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: Horizontal Line indention and outdention gives you a line that is not selectable → hr has duplicate lineboxes in frame tree
Target Milestone: --- → mozilla0.9.5
|
||
Comment 6•23 years ago
|
||
Eegad - what a mess! Yes, I see this, of course in Quirks mode only. In Quirks
Mode we treat HR as inline and use generated content to make it 'seem' like a
block. I wonder if this problem exists with other generated content as well...
I'll try to test that out.
I also wonder if that hack that buster put in so long ago to make HR act like
this in Quirks mode was worth it. If I remember correctly, it was to allow HR to
resize between floated elements.
|
|
||
Comment 7•23 years ago
|
||
So, If I create a document like:
<html>
<head>
<style>
span {
-moz-box-sizing: border-box;
margin: 0 0.1% 0 0.1%; /* Mmm! Hack-on-a-hack for bug 81776 */
}
span:before {
white-space: pre;
content: "\A";
}
span:after {
white-space: pre;
content: "\A";
}
</style>
</head>
<body>
One Line
<span>*</span>
Next Line
</body>
</html>
to emulate what we do with HR on a SPAN, I see the same problem with the
multiple line frames:
line 02962110: count=2 [0x2038] {0,0,848,285} <
Text(0)@0299AC74[0,10,T] next=0299AD40 {0,0,840,285} <
"\nOne Line\n"
>
Inline(span)(1)@0299AD40 next=02962244 next-in-flow=02962244 <
Inline(span)(1)@0299AE04 {0,225,0,0} [state=0000004c] <
Text(-1)@0299AE70[0,1,T] {0,-225,0,285} [state=00000044] <
"\n"
>
>
>
>
line 0296227C: count=1 state=inline,clean,NOT Impacted,<
Inline(span)(1)@02962244 next=029620D0 prev-in-flow=0299AD40 <
Text(0)@0299AEE4[0,1,T] next=02962024 {0,0,120,285} <
"*"
>
Inline(span)(1)@02962024 {120,225,0,0} [state=0000004c] <
Text(-1)@02962090[0,1,T] {0,-225,0,285} [state=00000044] <
"\n"
>
>
>
>
line 02962308: count=1 state=inline,clean,NOT <
Text(2)@029620D0[0,11,T] {0,570,900,285} [state=71000034] <
"\nNext Line\n"
>
>
Looking at this, I think it is correct. The HR (or SPAN in this case) have
generated content for the :before and :after style rules. So the extra lines
need to be there, and the extra frames in those lines have to be attributed to
the HR (or SPAN) tags. The problem of not removing them when the HR is deleted
though is a real bug. I bet that the problem of removing elements with generated
content is more general, not just restricted to HR elements (just a guess).
Attachment #48456 -
Attachment is obsolete: true
|
Assignee | |
Comment 10•23 years ago
|
||
|
|
Assignee | |
Comment 11•23 years ago
|
||
Marc was correct, the problem is not limited to HR. I just attatched a test case
that uses an image with :before and :after content that we can use to see that
the generated content frames aren't being removed.
It's important to note that things are deleted properly in Marc's test case
using the span, because it is splittable, so in that case the generated content
frames are in-flow as continuous frames for the span, and
nsBlockFrame::DoRemoveFrame() removes all continuing frames for content that is
removed.
The problem with inline-non-splittable elements is that their generated content
frames are siblings (not continuous frames), so when we remove the content node
from the tree, only the primary frame for that content node is removed.
There doesn't seem to be any notion of generated content inside nsBlockFrame, so
rather than introduce it, I decided to try and fix the problem in
nsCSSFrameConstructor.cpp, which is the same place that makes the generated
content a sibling of the primary frame.
I'll attatch the diff I used to fix the bug. The one thing I didn't like about
this approach was that in order to get the previous sibling for a frame, I had
to go up to the parent and walk it's child list till I got to the previous
sibling, which might be a performance problem on large flat pages.
I could try the approach of modifying nsBlockFrame::DoRemoveFrame() so that it
handles this situation, but I might have to complicate the loop logic there
because the generated content could be on different lines than the primary frame
etc.
Should I try that route? nsCSSFrameConstructor.cpp coming up.
|
|
Assignee | |
Comment 12•23 years ago
|
||
|
||
Comment 14•23 years ago
|
||
Looks fine to me, modulo stylistic vandalism (put the open braces on the same
line as the if/else/while, etc.)
HasPseudoStyle could just be a file-static (no need to add it to the class).
Also, why not just:
return (pseudoStyle != nsnull);
instead of creating a temporary, blah blah blah.
There isn't a better way to find the previous frame. Oh well.
You don't check |rv| after calling RemoveGeneratedContentSiblings: why even
bother assigning it to a temporary, then? (Why not make
RemoveGeneratedContentSiblings a |void| function.) And I guess this could just
be file-static, too (no need to add to the class).
|
|
Assignee | |
Comment 15•23 years ago
|
||
*** Bug 96453 has been marked as a duplicate of this bug. ***
|
|
||
Comment 16•23 years ago
|
||
kin, in case it wasn't clear, attachment 56089 [details] is okay by me module some
stylistic nits. Fix those, and sr=waterson.
|
|
Assignee | |
Comment 17•23 years ago
|
||
I just attatched a new patch incorporating all of waterson's suggestions. Can I
get an r= from attinasi or dbaron and an sr= from waterson?
Btw, this should fix waterson's bug 71979, but while using some of the test
cases in that bug, I think I ran across another bug where calling
GetPrimaryFrameFor() is returning the :before pseudo content frame for BR
content instead of the nsBRFrame for the BR. This results in only the :before
frame being removed leaving the nsBRFrame and any :after frame associated with
it in the frame tree.
Attachment #56098 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 18•23 years ago
|
||
Comment on attachment 57271 [details] [diff] [review]
Patch Rev 1.1 (With waterson's suggestions)
Marking sr=waterson per his comments above. :-)
Attachment #57271 -
Flags: superreview+
|
|
||
Comment 19•23 years ago
|
||
Looks good!
|
|
||
Comment 20•23 years ago
|
||
Comment on attachment 57271 [details] [diff] [review]
Patch Rev 1.1 (With waterson's suggestions)
These changes look good - r=attinasi
I wonder if
+ if (!aPresContext || !aPresShell || !aFrameManager || !aFrame)
+ return;
could just be an assertion though...
Attachment #57271 -
Flags: review+
|
|
Assignee | |
Comment 21•23 years ago
|
||
I changed:
+ if (!aPresContext || !aPresShell || !aFrameManager || !aFrame)
+ return;
to:
+ NS_ASSERTION(aPresContext && aPresShell && aFrameManager && aFrame, "Null arg
pointer!");
in my local tree.
Whiteboard: [EDITORBASE] FIX IN HAND, need r= → [EDITORBASE] FIX IN HAND, ready for checkin
|
||
Comment 22•23 years ago
|
||
No release needed for styleContext? Also, possible leak of styleContext in the
case where |if content && !content->IsContentOfType(nsIContent::eELEMENT) &&
styleContext| is true. Better to split this pattern (or nsCOMPtr<>).
+ nsCOMPtr<nsIContent> content;
+ nsIStyleContext *styleContext = nsnull;
+
+ aFrame->GetContent(getter_AddRefs(content));
+ aFrame->GetStyleContext(&styleContext);
+
+ if (!content || !content->IsContentOfType(nsIContent::eELEMENT) ||
!styleContext)
+ return NS_OK;
|
|
Assignee | |
Comment 23•23 years ago
|
||
Crap! Good catch rbs. I just attached Rev 1.2 that contains the assertion
attinasi wanted, and uses a COMPtr for the styleContext.
If there are no other objections, I'll check this in as soon as soon as I can
catch an open tree.
Attachment #57271 -
Attachment is obsolete: true
|
|
||
Comment 24•23 years ago
|
||
I am wondering if there could be several before/after frames? Is that possible?
e.g.
tag:before {
content: "foo";
content: "bar";
}
|
|
Assignee | |
Comment 25•23 years ago
|
||
I tried that when I first came up with the fix, but it didn't seem like you
could. The last content: you specified was what showed up on the screen and in
the frame tree.
|
|
Assignee | |
Comment 26•23 years ago
|
||
Patch Rev 1.2 checked into trunk:
mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp revision 1.655
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [EDITORBASE] FIX IN HAND, ready for checkin → [EDITORBASE] fixed on trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•