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

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
Editor
P2
major
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: sujay, Assigned: mjudge)

Tracking

Trunk
mozilla1.2alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] EDITORBASE)

Attachments

(1 attachment, 11 obsolete attachments)

13.38 KB, patch
Charles Manske
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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
(Reporter)

Comment 1

16 years ago
--> mjudge

this is a recent regression...I don't remember this happening before.
Assignee: cmanske → mjudge
(Reporter)

Comment 2

16 years ago
nominating
Keywords: nsbeta1
Whiteboard: EDITORBASE

Comment 3

16 years ago
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

16 years ago
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)
(Assignee)

Comment 5

16 years ago
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 ;)
Status: NEW → ASSIGNED
Was this a regression from bug 141054?
(Assignee)

Comment 7

16 years ago
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.
(Assignee)

Comment 8

16 years ago
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.
(Assignee)

Comment 9

16 years ago
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.
(Assignee)

Comment 10

16 years ago
Created attachment 92825 [details] [diff] [review]
nsSelection.cpp patch for HRFrames
(Assignee)

Comment 11

16 years ago
Created attachment 92827 [details] [diff] [review]
nsFrame.cpp patch for HRFrames
(Assignee)

Updated

16 years ago
Attachment #92827 - Attachment description: patch for HRFrames. see above for comments → nsFrame.cpp patch for HRFrames
(Assignee)

Comment 12

16 years ago
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
(Assignee)

Comment 13

16 years ago
Created attachment 92829 [details] [diff] [review]
nsHRFrame.cpp fix for HRFrames

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

Comment 14

16 years ago
Created attachment 92831 [details] [diff] [review]
HRFrame patch 2.  better drawing i think

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

Updated

16 years ago
Attachment #92829 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Created attachment 92850 [details] [diff] [review]
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.
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).
(Assignee)

Comment 17

16 years ago
Created attachment 92851 [details] [diff] [review]
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.
Attachment #92825 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
Created attachment 92857 [details] [diff] [review]
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.
Attachment #92850 - Attachment is obsolete: true
(Assignee)

Comment 19

16 years ago
Created attachment 92929 [details] [diff] [review]
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.
Attachment #92857 - Attachment is obsolete: true
(Assignee)

Comment 20

16 years ago
Created attachment 92935 [details] [diff] [review]
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
Attachment #92831 - Attachment is obsolete: true

Comment 21

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

sr=kin@netscape.com 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

16 years ago
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

16 years ago
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

16 years ago
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.
(Assignee)

Comment 25

16 years ago
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
http://mozilla.org/docs/web-developer/quirks/ for more info on modes (in
particular, how to trigger the different modes).

Comment 27

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

r=jfrancis
Attachment #92851 - Flags: review+

Updated

16 years ago
Attachment #92929 - Flags: review+

Updated

16 years ago
Attachment #92935 - Flags: review+

Comment 28

16 years ago
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

16 years ago
Created attachment 93097 [details] [diff] [review]
fixed missing brackets for if: nsSelection.cpp

Found a crasher in testing.  An if clause was missing it's brackets.
Attachment #92851 - Attachment is obsolete: true

Updated

16 years ago
Attachment #93097 - Flags: review+
(Assignee)

Comment 30

16 years ago
Created attachment 93170 [details] [diff] [review]
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.
Attachment #92935 - Attachment is obsolete: true

Comment 31

16 years ago
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.
(Assignee)

Comment 32

16 years ago
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+
(Assignee)

Comment 33

16 years ago
Created attachment 93443 [details] [diff] [review]
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.
Attachment #92929 - Attachment is obsolete: true
Attachment #93097 - Attachment is obsolete: true
Attachment #93170 - Attachment is obsolete: true

Updated

16 years ago
Blocks: 95654

Comment 34

16 years ago
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=kin@netscape.com
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);
Attachment #93443 - Flags: superreview+
(Assignee)

Comment 35

16 years ago
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

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

r=cmanske
Attachment #93443 - Flags: review+
(Assignee)

Comment 37

16 years ago
need approval i guess. this for branch trunk or wha?
(Assignee)

Comment 38

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

Comment 39

16 years ago
fixing summary since this bug covers the selection/hilite issue as well as caret
problem
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)
(Assignee)

Comment 40

16 years ago
checked into trunk

Comment 41

16 years ago
sujay: can you pls verify this as fixed on the trunk? thanks!
Severity: normal → major
Keywords: nsbeta1 → approval, mozilla1.2, nsbeta1+
Priority: -- → P2
Whiteboard: EDITORBASE → [adt2] EDITORBASE
Target Milestone: --- → mozilla1.2alpha
(Reporter)

Comment 42

16 years ago
verified on 8/7 trunk.
(Assignee)

Comment 43

16 years ago
marking this fixed. if anyone wants it on a trunk lemme know.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 44

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

Comment 45

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

Updated

16 years ago
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.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.