We can select HR in browser by single-click. In editor, this behavior is ok. But in browser it is strange. Probably, this bug is the effect of Bug 159207. It seems natural in UI if we can select HR by *double-click* like a word. I confirmed on trunk build 2002081908/Linux.

If it's happening in the browser, it's not an editor bug. --> mjudge (selection) I know that mjudge made changes that moved hr selection under control of the text selection display flag which is set in the browser.

--> pmac

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

this is a regression. I am running build 20020721 on Windows XP and the said behaviour doesn't seem to reproduce.

from Asa in #mozillazine, we isolted the issue as bug occur in build 20020827 and not 20020824.

Possible culprit: bug 4302 (modified selection code). CC'ing people that produced the patch.

> from Asa in #mozillazine, we isolted the issue as bug occur in build 20020827 > and not 20020824. sorry, no. regression between 2002080508 and 2002080708. bug 159207 looks like a better culprit

This one is real annoying. Just try to select text at a news site or Mozillazine and it crops up.

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

In answer to Comment #4, the bug seem to be reproducable every time in WinXP as well. Anyone who have the edit permission... Please add the 'horizontal ruler' to the summary textbox, starting after 'HR' and before 'is highlighted by single click in browser'. This will yield a better search result in bugzilla search. I never found this bug when searching bugzilla and ended up filing on a bug that end up as a duplicate bug. Thanks.....

Nominating for 1.3b.

check for DISPLAY_FRAMES not DISPLAY_TEXT during paint of HR

The hr hiliting in the browser was introduced by mjudge in bug 159207. I asked him to make the change/restore the use of DISPLAY_FRAMES, which marcl does in this patch, in bug 159207 comment 34, but in comment 35 he said he wanted to keep that change. Also note that all this change does is keep the HR from drawing hilited when it is selected, that is, it still gets selected when you click on it, there just isn't any visual feedback, which is how it worked in the moz1.0 timeframe. Note that in IE, hrules get hilited when they are in a selected range, but if you click on them, they are not hilited. If people really want the old behavior, then I'm ok with this change (r+sr=kin@netscape.com), I just wanted everyone to be aware that this merely changes the visual aspect of what gets selected when you click on an HR.

I think it will be fine if a single click still selects the HR, as long as it doesn't visually highlight the HR.

I think it's very odd not to show that there is a selection when there is one.

Then don't select HR on a single click. Either that or don't show the selection. Selecting an HR with a single click and highlighting it is not good, it is very distracting and annoying.

Sorry. All I mean is that for someone like me, who does a lot of clicking within web pages to select text, this bug is annoying. Couldn't we allow a single click on an HR to do nothing, but have a double-click select the HR?

Can someone checkin the r+sr reviewed patch?

The attached patch has not been reviewed or super-reviewed. Only requests for reviews have happened. Of those requests, neither is a super-reviewer. One person is on extended vacation. The other person is on medical leave. One question about this patch, has it been tested in composer? In composer, if I click on the HR, I expect to see a box around it (or some indication that I can delete it). Is this the case?

I assumed that r+sr=kin in comment 13 was sufficient. Yes, the patch was tested with composer. The behavior in composer remains unchanged.

I agree with comment #17. It is good to highlight the HR when it is selected, but it should require a double-click to be selected.

Comment on attachment 111572 [details] [diff] [review] Patch that corrects the problem please don't request approval for patches that haven't been reviewed. thanks.

I haven't taken a look at the C Programming in Mozilla to know if this will be affected or not because I'm not familiar with lengthly C Programming. I did take a look at the patch. So, I'll summarize it all. Suppose there is text before and after the HR. If the user drag the mouse across the text and goes over the HR and continue on dragging the text. Will the HR continue to be highlighted without a problem? This is a feature that should be working without a problem. Or the purpose of the HR lighlighting will be defeated. Can someone check this out with the patch?? Is it a problem??

This is an annoying problem for the many users who click on web pages and select text while browsing. It should be fixed for 1.4 alpha.

See also bug 12764. One could argue that this is a dupe of it.

If this bug is actually a duplicate of bug #12764 when how does this bug #159207 as shown in comment #13 of this bug report...

Don't select an HR when clicking on it. Behavior in editor mode remains unchanged.

Comment on attachment 117161 [details] [diff] [review] patch v2 I'm assuming that this functions correctly (doesn't select on single click, clearly, but _does_ select on double click). That said, I realize that you're just following what the existing code does, but I don't like what the existing code does (not to mention that the existing code for <area> is just plain incorrect). Perhaps something more like: if (content->IsContentOfType(nsIContent::eHTML)) { nsCOMPtr<nsIAtom> tag; content->GetTag(*getter_AddRefs(tag)); // Check tag here, call GetAttr as needed to get attributes (eg both nsHTMLAtoms::area and nsHTMLAtoms::a will use the nsHTMLAtoms::href attribute). } Make sense? The idea is to avoid expensive QI misses and replace them with the much cheaper pointer comparisons that atom equality comparison turns into.

To Marc, since he's the one working on this. Sorry to make you clean up this bit of code, but _someone_ has to do it. Plus, you'll be fixing two bugs with one stone (the <area> thing).

Double-click on HR doesn't select. I could make the changes to do that but in my opinion selecting a single HR does not have very much value in Browser. You could drag over it to select if necessary.

OK. I'm fine with that if the selection people (brade? mjudge?) are.

It was broken before when not selecting hrules at all. Now it is fixed so its called a regression. Honestly I cant see that there are THAT many hrules on webpages that this warrants being called a big problem. Given the few number of webpages that even use this tag any more, given that the square area of each webpage relative to an hrule and the few amount of people that could accidentally click on an hrule while trying to navigate and be that upset, I dont see why we need to special case hrules. We may not behave completely like I.E. on this issue. So what. We are clicking on something I liken to an image. When we click on images we select them. That being said I really dont care about this that much. As far as the patch goes, the LAST thing we want is for people who wish to start a selection to accidentally click on an hrule and have the mousedown thrown out. Instead you must override GetContentAndOffsetsFromPoint for HRules. Make nsHRule::GetContentAndOffsetsFromPoint and have it see where they clicked and stick a collapsed selection in front of or behind (depending on geometry of click). Right now nsHRule uses the default behaviour of all other frames in nsFrame::GetContentAndOffsetsFromPoint which, if the point is contained within the frames rectangle, will extend the selection end offset to +1 relative to beginning offset. Putting this into HRuleFrame will be much better as it moves the special case to inheiritance inside the nsHRFrame.cpp file instead of in some random place in nsFrame::HandlePress. Oh one more thing, try using diff -u10 to get more context on a patch. it was hard for me initially to tell what method the patch was changing.

> Honestly I cant see that there are THAT many hrules on > webpages that this warrants being called a big problem. I don't think this is an important bug. However, it is still a bug and it should still be fixed if we have someone volunteering to fix it. > When we click on images we select them. Actually we don't, and that's good.

There are a lot of hrules on sites that I frequent. Mainly news sites and web logs. I will be pleased as punch when this gets fixed, believe me. Go, Marc, go!

Implemented mjudge's suggestion to override GetContentAndOffsetsFromPoint in nsHRuleFrame.cpp

I'm not really familiar with the code in question, and I'm sorta in the middle of a real-life time crunch. I will not be able to review this patch for at least a few days; if I don't get to it by Friday, then not for another week after that.

Comment on attachment 117349 [details] [diff] [review] Patch v3 mjudge is not an sr...

Comment on attachment 117349 [details] [diff] [review] Patch v3 A couple of nits: > nsIPresContext* aCX, This is usually called "aPresContext" > nsresult result should be "rv" > if (*aNewContent){ make it "if (!*aNewContent) return rv;" > //temp to hold old value in case of failure Why do you have to do this? Are there callers who rely on the value not changing in case of failure? + if (NS_FAILED(result) || contentOffset < 0) + { + return (result?result:NS_ERROR_FAILURE); + } if (NS_FAILED(rv)) return rv; if (contentOffset < 0) return NS_ERROR_FAILURE; Fix those and answer the question, then you're good to go.

Regarding the temp, most of the code was copied from nsFrame::GetContentAndOffsetsFromPoint() which is more general. My testing determined that the temp is not required in nsHRFrame::GetContentAndOffsetsFromPoint() so I removed it. Robert, I addressed your nits. r/sr?

Can someone check-in the patch? I don't have cvs access.

checked in: Checking in nsHRFrame.cpp; /cvsroot/mozilla/layout/html/base/src/nsHRFrame.cpp,v <-- nsHRFrame.cpp new revision: 1.63; previous revision: 1.62 done however, I had to fix cvs conflicts, I'll attach the patch as I checked it in

YES! Thank you, thank you, thank you! You guys rock.