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.

nominating

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

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

Was this a regression from bug 141054?

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 properly. patches to follow.

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.

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

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

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.

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.

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

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 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 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?

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.

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 on attachment 92851 [details] [diff] [review] nsSelection.cpp patch forgot to add this to patches. fixes check for non-null sibling r=jfrancis

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.

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

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.

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.

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

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. r=cmanske

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 problem

checked into trunk

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

verified on 8/7 trunk.

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

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

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