163594 - HR is highlighted by single click in browser (horizontal ruler) (select)

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Shotaro Kamio]

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.

Comment 1

[kinmoz]

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.

Comment 2

[sujay]

--> pmac

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Manoj]

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

Comment 5

[Olivier Cahagne]

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

Comment 6

[Olivier Cahagne]

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

Comment 7

[Andrew Schultz]

> 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

Comment 8

[Andrew Hagen]

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

Comment 9

[R.K.Aa.]

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

Comment 10

[Zook Valem]

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

Comment 11

[Andrew Hagen]

Nominating for 1.3b.

Comment 12

[Marc Loiselle]

Attachment: Patch that corrects the problem

check for DISPLAY_FRAMES not DISPLAY_TEXT during paint of HR

Comment 13

[kinmoz]

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=[e-mail]), I just wanted everyone to be aware that this merely
changes the visual aspect of what gets selected when you click on an HR.

Comment 14

[Andrew Hagen]

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

Comment 15

[Kathleen :Brade]

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

Comment 16

[Andrew Hagen]

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. 

Comment 17

[Andrew Hagen]

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?

Comment 18

[Marc Loiselle]

Can someone checkin the r+sr reviewed patch?

Comment 19

[Kathleen :Brade]

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?

Comment 20

[Marc Loiselle]

I assumed that r+sr=kin in comment 13 was sufficient.  

Yes, the patch was tested with composer.  The behavior in composer remains
unchanged.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 22

[Asa Dotzler [:asa]]

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.

Comment 23

[Zook Valem]

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

Comment 24

[Andrew Hagen]

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. 

Comment 25

[Vaclav Dvorak]

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

Comment 26

[Sam J. Fleet]

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

Comment 27

[Marc Loiselle]

Attachment: patch v2

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Marc Loiselle]

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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[mjudge]

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.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 34

[Andrew Hagen]

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!

Comment 35

[Marc Loiselle]

Attachment: Patch v3

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117349 [details] [diff] [review]
Patch v3

mjudge is not an sr...

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 39

[Marc Loiselle]

Attachment: Patch v3.1

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?

Comment 40

[Marc Loiselle]

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

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch as checked in

Comment 43

[Andrew Hagen]

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