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

Status: VERIFIED FIXED

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

Description

[sujay]

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

Comment 1

[sujay]

--> mjudge

this is a recent regression...I don't remember this happening before.

Comment 2

[sujay]

nominating

Comment 3

[Charles Manske]

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)

Comment 4

[Charles Manske]

This is probably unrelated, but is bug 155656 is another bad HR bug (doesn't
show selection highlighting.)

Comment 5

[mjudge]

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 ;)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Was this a regression from bug 141054?

Comment 7

[mjudge]

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.

Comment 8

[mjudge]

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.

Comment 9

[mjudge]

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 
properly. 

patches to follow.

Comment 10

[mjudge]

Attachment: nsSelection.cpp patch for HRFrames

Comment 11

[mjudge]

Attachment: nsFrame.cpp patch for HRFrames

Comment 12

[mjudge]

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.

Comment 13

[mjudge]

Attachment: nsHRFrame.cpp fix for HRFrames

draw selected hrframes properly. check the parent to see if we are selected.

Comment 14

[mjudge]

Attachment: HRFrame patch 2. better drawing i think

better drawing of the rectangle. draws including borders so you can see smaller
hrframes better

Comment 15

[mjudge]

Attachment: adds a better handling of clicking/end of line on lines that end in frames with 0 width

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.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 17

[mjudge]

Attachment: nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling

Forgot to check for non null frame after getting next in flow.

Comment 18

[mjudge]

Attachment: nsFrame.cpp adds check to make sure arrowing around will not get stuck.

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.

Comment 19

[mjudge]

Attachment: nsFrame.cpp with fix for & on bit comparison 'doh

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.

Comment 20

[mjudge]

Attachment: nsHRFrame.cpp needed to take into account turning off of 3d style on hr rules

just moved the selection drawing to outside the if (drawstyle...) block

Comment 21

[kinmoz]

Comment on attachment 92851 [details] [diff] [review]
nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling

sr=[e-mail] 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
ava
ilable sibling of selection status.


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


+	   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 22

[kinmoz]

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:


NS_IMETHODIMP
 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 23

[kinmoz]

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) ==
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,
selectionBGColor);
+	    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,
height);
+  }


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?

Comment 24

[Charles Manske]

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:       "http://www.w3.org/TR/html4/strict.dtd"
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.

Comment 25

[mjudge]

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's correct that the wrapper will only exist in quirks mode.  See
http://mozilla.org/docs/web-developer/quirks/ for more info on modes (in
particular, how to trigger the different modes).

Comment 27

[Joe Francis]

Comment on attachment 92851 [details] [diff] [review]
nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling

r=jfrancis

Comment 28

[Joe Francis]

review comments:

nsSelection.cpp

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

--------------

nsFrame.cpp

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.

--------------

nsHRFrame.cpp

i don't grok why nsFrame::Paint is no longer called from HRuleFrame::Paint.

Comment 29

[Joe Francis]

Attachment: fixed missing brackets for if: nsSelection.cpp

Found a crasher in testing.  An if clause was missing it's brackets.

Comment 30

[mjudge]

Attachment: nsHRFrame.cpp fix for DTD changes

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.

Comment 31

[Charles Manske]

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 32

[mjudge]

Comment on attachment 93170 [details] [diff] [review]
nsHRFrame.cpp fix for DTD changes

marking as reviewed for charlie. just need SR now.

Comment 33

[mjudge]

Attachment: one patch for 3 files.

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.

Comment 34

[kinmoz]

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 sr=[e-mail]
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
   etc.


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

Comment 35

[mjudge]

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 36

[Charles Manske]

Comment on attachment 93443 [details] [diff] [review]
one patch for 3 files.

r=cmanske

Comment 37

[mjudge]

need approval i guess. this for branch trunk or wha?

Comment 38

[mjudge]

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

Comment 39

[Kathleen :Brade]

fixing summary since this bug covers the selection/hilite issue as well as caret
problem

Comment 40

[mjudge]

checked into trunk

Comment 41

[Jaime Rodriguez, Jr.]

sujay: can you pls verify this as fixed on the trunk? thanks!

Comment 42

[sujay]

verified on 8/7 trunk.

Comment 43

[mjudge]

marking this fixed. if anyone wants it on a trunk lemme know.

Comment 44

[mjudge]

i mean if anyopne wants it checked in on a branch let me know. duh..

Comment 45

[Charles Manske]

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

Comment 46

[sairuh (rarely reading bugmail)]

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.)