Closed Bug 159207 Opened 22 years ago Closed 22 years ago

unable to select hrule; caret jumps to middle and blinks after inserting H.Line (HR, horizontal line)


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






(Reporter: sujay, Assigned: mjudge)



(Whiteboard: [adt2] EDITORBASE)


(1 file, 11 obsolete files)

13.38 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
using 7/24 branch build

1) launch netscape
2) Insert | Horizontal Line

notice the caret jumps to the middle and is blinking.

this doesn't seem right visually. Maybe the caret should be at the end
of the H. Line. or at the next line.

this could be related to bug 98619
--> mjudge

this is a recent regression...I don't remember this happening before.
Assignee: cmanske → mjudge
Keywords: nsbeta1
Whiteboard: EDITORBASE
HRs are totally horked!
Besides this problem, there are about 40 bugs relating to HR!
Bug 158197 (align ignored) is really bad!
These might be related:
bug 68619 (can't click to position caret near HR),
bug 95654 seems related (bad caret after backspace)
bug 96644 (can't place text to left of HR)
bug 98552 (down arrow [and right arrow now?] don't move caret location)
This is probably unrelated, but is bug 155656 is another bad HR bug (doesn't
show selection highlighting.)
Summary: caret jumps to middle and blinks after inserting H.Line → caret jumps to middle and blinks after inserting H.Line (HR, horizontal line)
looked into this and talked about it with composer group.  I am working on a fix 
to this odd behavior.  The inline splittable frame has been changed around the 
hr frame and so these bugs have popped up.  I will attempt a few things and send 
dbaron a ping to see if I can bother him on vacation ;)
yes its a regression of that.  Does not crash ,but makes sensible use of HRs in 
mail or composer impossible. caret drawn in odd place and no blue selection.  Is 
it possible for me to talk to you for a few min just to give you my ideas?  if 
not, thats cool I will just try to write them up.
I have code that works now in 90% of the cases for caret placement and blue 
highlighting. I will make a patch and let people take a look. Its a little hacky 
but it is the best generic response to a hack I could come up with.
ok redid some of patch I think this is a good fix.  now any frame that is 
selected that has children that overflow it will also tell the children to 
invalidate as well.  This will help other cases of selection leaving things 
undrawn.  Fixes the caret movement by skipping lines that are of 0 height. 
Fixes getting incorrect frame while looking for frame of a certain offset (aka 
caret placement) to skip frames that are of 0 width and have a 
frame "nextinflow".

I also draw the hrule selected by filling in the area inside the hrule with the 
select color.  This looks fine and allows for show all tags mode to work 

patches to follow.
Attached patch nsFrame.cpp patch for HRFrames (obsolete) — Splinter Review
Attachment #92827 - Attachment description: patch for HRFrames. see above for comments → nsFrame.cpp patch for HRFrames
Comment on attachment 92825 [details] [diff] [review]
nsSelection.cpp patch for HRFrames

to notify of a selection, if the rectangle of the first frame in flow is 0
width or 0 height,to set the next frame in the flow.
Attachment #92825 - Attachment description: patch for HRFrames. see above for comments → nsSelection.cpp patch for HRFrames
Attached patch nsHRFrame.cpp fix for HRFrames (obsolete) — Splinter Review
draw selected hrframes properly. check the parent to see if we are selected.
better drawing of the rectangle. draws including borders so you can see smaller
hrframes better
Attachment #92829 - Attachment is obsolete: true
this just makes it so that if you put an HR on the same line as some text, even
though it appears to wrap, the inline frame of 0width and with the content of
the HRule is on the line with the text.  So when you click to the right of it
or hit eol you end up to the left of the HRule.  This also has the effect of
fixing another of joe's bugs of clicking to the right of BRs puts the caret
outside of the style sometimes.
Attachment #92827 - Attachment is obsolete: true
Comment on attachment 92850 [details] [diff] [review]
adds a better handling of clicking/end of line on lines that end in frames with 0 width

>+  if (frameState | NS_FRAME_OUTSIDE_CHILDREN)
>+  {
>+    RefreshAllContentFrames(aPresContext,this,mContent);
>+  }

This should be &, not |, and you might want to check the bit at every level of
the recursion, i.e., within the RefreshAllContentFrames function (although the
content check should make it mostly irrelevant).
Forgot to check for non null frame after getting next in flow.
Attachment #92825 - Attachment is obsolete: true
make sure that when we arrow around we dont get stuck. in this case we could
get stuck on right arrow on an hrule (perhaps something else like this as well)
where getting the next frame returns us to the same content so we cannot right
arrow through the whole document.  if we get back a frame who's content is the
same as ours and we are not skipping to the next line then we have 2 frames
next to each other that point to the same. In the case of a 2 framed CSS styled
text frame we of course must have a special case.
Attachment #92850 - Attachment is obsolete: true
fixes the humiliating | comparison.  I didnt make the original static methods
to refresh all content belonging to a frame and its children.  I am not sure
what the method was used for but I figure not checking each framestate is'nt
that bad since as dbaron says the content check eliminates 99% of redundancy.
Attachment #92857 - Attachment is obsolete: true
just moved the selection drawing to outside the if (drawstyle...) block
Attachment #92831 - Attachment is obsolete: true
Comment on attachment 92851 [details] [diff] [review]
nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling with the following conditions:

You can get rid of this comment after frameRect:

+    nsRect frameRect; //if a rect is 0 height/width then try to notify next
ilable sibling of selection status.

And move the comment, below this code you added, so that it comes before the

+	   frame->GetRect(frameRect);
+	   if (!frameRect.width || !frameRect.height)
+	   {
+	      //try to notify next in flow that its content is selected.
+	     if (NS_SUCCEEDED(frame->GetNextInFlow(&frame)) && frame)
+	       frame->SetSelected(aPresContext, nsnull,aFlags,eSpreadDown);
+	   }
+	 //if the frame is splittable and this frame is 0,0 then set the
sibling frame to be selected.

Also, to avoid confusion the comment should refer to "next-in-flow" rather than
"sibling frame" since they are different.

Also, please augment the comment to give an example where this situation can
come about? It can help in the future if someone else is debugging or looking
at this code.

Do we ever need to worry if the next-in-flow also has zero width or height?
Comment on attachment 92851 [details] [diff] [review]
nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling

Please add a comment that describes a scenario you are trying catch, this will
help others in the future understand what you are doing:

 nsFrame::GetChildFrameContainingOffset(PRInt32 inContentOffset, PRBool inHint,
PRInt32* outFrameContentOffset, nsIFrame **outChildFrame)
   NS_PRECONDITION(outChildFrame && outFrameContentOffset, "Null parameter");
   *outFrameContentOffset = (PRInt32)inHint;
+  nsRect rect;
+  GetRect(rect);
+  if (!rect.width || !rect.height)
+  {
+    nsIFrame *nextFlow = nsnull;
+    if (NS_SUCCEEDED(GetNextInFlow(&nextFlow)) && nextFlow)
+      return nextFlow->GetChildFrameContainingOffset(inContentOffset, inHint,
outFrameContentOffset, outChildFrame);
+  }

Now that you are removing the reference to <br> in this change, please add a
comment here that gives an example of the types of frames you expect to trigger
this condition:

-    nextFrame->GetFrameType(getter_AddRefs(frameType));
-    if (nsLayoutAtoms::brFrame == frameType.get()) 
+    nextFrame->GetRect(rect);
+    if (!rect.width) 

A comment with an example illustrating the situation you are trying to avoid
here would be helpful.

+  nsIContent *content = nsnull;
+  newFrame->GetContent(&content); 
+  if (!lineJump && (content == mContent))
+  {
+    //we will continue if this is NOT a text node. 
+    //in the case of a text node since that does not mean we are stuck. it
could mean a change in style for
+    //the text node
+    nsCOMPtr<nsIAtom> frameType;
+    newFrame->GetFrameType(getter_AddRefs(frameType) );
+    if (nsLayoutAtoms::textFrame != frameType.get() )
+      continue;  //we should NOT be getting stuck on the same piece of content
on the same line. skip to next line.
+  }
Comment on attachment 92935 [details] [diff] [review]
nsHRFrame.cpp needed to take into account turning off of 3d style on hr rules

If this code is assuming that the parent is the "wrapper" frame that dbaron
added, I think this "wrapper" frame might not exist in HTML pages that invoke
Standards mode. Is that correct dbaron? If that's right, this may cause the hr
to look selected when it isn't right?

+  nsIFrame *parent = nsnull;
+  //hack to get around lack of selection bit setting.
+  GetParent(&parent);//get the parent and check to see if its selected
+  PRBool isSelected = PR_FALSE;
+  if (parent)
+  {
+    nsFrameState  frameState;
+    parent->GetFrameState(&frameState);
+    isSelected = (frameState & NS_FRAME_SELECTED_CONTENT) ==
+  }

The indention here needs fixing:

+  nscolor selectionBGColor = 0;
+  if (isSelected)
+  {
+	  nsILookAndFeel* look = nsnull;
+	  if (NS_SUCCEEDED(aPresContext->GetLookAndFeel(&look)) && look) {
+	    look->GetColor(nsILookAndFeel::eColor_TextSelectBackground,
+	    NS_RELEASE(look);
+	  }

Should we be using the (x,y) value from GetRect() like your nsFrame changes do
instead of (0,0) here?

+    aRenderingContext.SetColor (selectionBGColor);
+    aRenderingContext.FillRect(0,0,mRect.width,mRect.height);//(x0, y0, width,
+  }

Also, nsFrame::Paint() does some checking to see if it should draw selection
based on the SelectionDisplay flags, no checking is done here ... it should be
checked here too right?
I tested all 3 patches. It was very good of Kin to point out DTD differences;
I tested 
standard:     "-//W3C//DTD HTML 4.01//EN"
strict:       ""
transitional: "-//W3C//DTD HTML 4.01 Transitional//EN"
All arrow navigation problems are fixed for all 3 DTDs.
The display of selection highlighting only works in the "transitional" DTD.
Interestingly, the alignmen not working bug 158197 only exists in transitional DTD.
hmm ok I can check to see if the parents content is the same. if not then I 
will look at the frame's own bit. I believe i have fixes for all of your 
concerns kin. I dont know how tabs got into this file to begin with I think it 
was left over from before.
It's correct that the wrapper will only exist in quirks mode.  See for more info on modes (in
particular, how to trigger the different modes).
Comment on attachment 92851 [details] [diff] [review]
nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling

Attachment #92851 - Flags: review+
Attachment #92929 - Flags: review+
Attachment #92935 - Flags: review+
review comments:


what if you have more than one zero-width frame in a row?  will we only
select the secons one? is that right?



although i bet its not supposed to happen, i can find no guarantee that 
GetNextInFlow cant return the same frame.  GetChildFrameContainingOffset()
will have an infinite loop if this happens.  

DrillDownToEndOfLine(): now that you check for zero size frames insteadd of br's, 
is it possible that multiple zero width frames will be in a row?  i can't tell
if this routine will do the right thing there.

i have no idea whats going on in GetFrameFromDirection(), so sr should pay close
attention there.



i don't grok why nsFrame::Paint is no longer called from HRuleFrame::Paint.
Found a crasher in testing.  An if clause was missing it's brackets.
Attachment #92851 - Attachment is obsolete: true
Attachment #93097 - Flags: review+
this should now look at nsHRFrame's framestate if the parent's content is not
the same as its own.  This should allow for any DTD that strips away the
default wrapper frame.
Attachment #92935 - Attachment is obsolete: true
I retested all 3 DTDs (standard 4.01, transitional 4.0, and strict) and with the
nsHRFrame.cpp patch (7-29), selection works great for all of them.
Comment on attachment 93170 [details] [diff] [review]
nsHRFrame.cpp fix for DTD changes

marking as reviewed for charlie. just need SR now.
Attachment #93170 - Flags: review+
just to be sure its all good and all comments and changes are in i am reposting
with the last comment change. This is all 3 files so we dont have to deal with
3 patches.
Attachment #92929 - Attachment is obsolete: true
Attachment #93097 - Attachment is obsolete: true
Attachment #93170 - Attachment is obsolete: true
Blocks: 95654
Comment on attachment 93443 [details] [diff] [review]
one patch for 3 files.

The latest round of changes look ok to me, so I'll give an
with the following addressed:

== nsHRFrame.cpp

-- So HR selection is always on if we're doing text selection? According
   to the comments in nsISelectionDisplay.idl you should be checking to
   see if DISPLAY_FRAMES is set instead of DISPLAY_TEXT:

    +	 if (!(displaySelection & nsISelectionDisplay::DISPLAY_TEXT))

-- Why is the (x,y) here (0,0)? Should it be based on the (x,y) position of
   the frame like it is in nsFrame::Paint()? What's the coordinate system
   being used? I just want to make sure this draws correctly if we're scrolled

+    aRenderingContext.FillRect(0,0,mRect.width,mRect.height);//(x0, y0, width,
Attachment #93443 - Flags: superreview+
I wanted the HR to act like a etxt frame and draw in the browser. It looks cool 
and its not a problem like the placeholder frames people use and you get blue 
boxes all over your webpage.  This goes against the TEXT_SELECTION and 
FRAME_SELECTION paradigm but I think its worth it. 

the drawing location of 0,0 is because the actual rect of an HR is higher than 
it should be in the negative y coordinates. so when you draw up there you end 
up drawing inside the line above the hr and not far enough below it.  0,0 looks 
completely correct.
Comment on attachment 93443 [details] [diff] [review]
one patch for 3 files.

Attachment #93443 - Flags: review+
need approval i guess. this for branch trunk or wha?
*** Bug 155656 has been marked as a duplicate of this bug. ***
fixing summary since this bug covers the selection/hilite issue as well as caret
Summary: caret jumps to middle and blinks after inserting H.Line (HR, horizontal line) → unable to select hrule; caret jumps to middle and blinks after inserting H.Line (HR, horizontal line)
checked into trunk
sujay: can you pls verify this as fixed on the trunk? thanks!
Severity: normal → major
Priority: -- → P2
Whiteboard: EDITORBASE → [adt2] EDITORBASE
Target Milestone: --- → mozilla1.2alpha
verified on 8/7 trunk.
marking this fixed. if anyone wants it on a trunk lemme know.
Closed: 22 years ago
Resolution: --- → FIXED
i mean if anyopne wants it checked in on a branch let me know. duh..
*** Bug 67838 has been marked as a duplicate of this bug. ***
Blocks: 163631
really marking vrfy'd for the trunk. (tested w/2003.02.19: after inserting and
HR, the cursor moves to the end (right side) of the HR.)
You need to log in before you can comment on or make changes to this bug.