The default bug view has changed. See this FAQ.

"Highlight All" find selection not visible in elements that have -moz-user-select: none set

VERIFIED FIXED in mozilla1.9.1b1

Status

()

Core
Selection
--
minor
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: graememcc)

Tracking

({testcase})

Trunk
mozilla1.9.1b1
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 334555 [details]
testcase

Again, formally a regression, but practically this is only happening because of the switch to the use of selection.
This already didn't work for the normal find.

To reproduce:
- Ctrl-f, type in the word 'mozilla'
- Click on "Highlight All"

Only the first 'mozilla' word is highlighted.

It seems to me -moz-user-select should only have an effect on the normal selection, not on any other selection type.
(Assignee)

Comment 1

9 years ago
So, that'll be the IsSelectable computations in the SetSelected method of the relevant frames stopping this working.

> It seems to me -moz-user-select should only have an effect on the normal
> selection, not on any other selection type.

To achieve this, I'm imagining this would require passing in the selection type as a parameter to the frame SetSelected methods. Yuck.

roc, any thoughts?
That sounds right. Is it really yuck?
Or, would it make sense to just ignore IsSelectable in the SetSelected methods? It's OK to invalidate and/or reflow unnecessarily. The only important thing is that we don't paint the selection if it's not supposed to be selectable.
(Assignee)

Comment 4

9 years ago
Created attachment 335301 [details] [diff] [review]
Patch v1

> Is it really yuck?

OK, maybe I was being a little dramatic :)

Just seems to be a lot of extra parameters flying all over the place, for something that's a bit of an edge-case.

But here we are...
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Attachment #335301 - Flags: superreview?(roc)
Attachment #335301 - Flags: review?(roc)
What do you think about comment #3 though?
(Assignee)

Comment 6

9 years ago
> What do you think about comment #3 though?

I think I don't know enough about the painting code to answer that intelligently! I'll spend a bit of time this week having a look around, and collecting my thoughts.

For the moment:

Making that change, what does the NS_FRAME_SELECTED_CONTENT flag mean? At the moment it means a region of the frame is selected, and that the selection acting on it will be displayed. The comment 3 approach will make it mean that a region of the frame is selected, which may or may not be displayed. This seems less useful, and likely to catch people out in future

Code such as 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1888 
and possibly 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4049 
go from being simple flag tests to presumably having to compute/look up style rules.
Comment on attachment 335301 [details] [diff] [review]
Patch v1

This seems reasonable. It actually seems kinda weird to pass in the range and the selection state without telling the frame what selection is actually involved.

The toolkit changes don't seem to be part of this patch, though, so you should resubmit a new patch for checkin without them.
Attachment #335301 - Flags: superreview?(roc)
Attachment #335301 - Flags: superreview+
Attachment #335301 - Flags: review?(roc)
Attachment #335301 - Flags: review+
I think it does make sense to change NS_FRAME_SELECTED_CONTENT to mean just a hint "something in this frame may be selected", but you're right, this isn't necessarily the right time to do that.
(Assignee)

Comment 9

9 years ago
Created attachment 335566 [details] [diff] [review]
Patch v2 - now with less cross-bug pollution
[Checkin: Comment 10]

> The toolkit changes don't seem to be part of this patch
And that's why you shouldn't work with patch queues when you're tired, lest you forget a "qpop" along the way. Apologies!

Carrying forward r+sr=roc
Attachment #335301 - Attachment is obsolete: true
Attachment #335566 - Flags: superreview+
Attachment #335566 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 335566 [details] [diff] [review]
Patch v2 - now with less cross-bug pollution
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/6cbd63d2a897
Attachment #335566 - Attachment description: Patch v2 - now with less cross-bug pollution → Patch v2 - now with less cross-bug pollution [Checkin: Comment 10]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
(Reporter)

Comment 11

9 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080908091724 Minefield/3.1b1pre

I just filed bug 454790 for the fact that normal find doesn't highlight -moz-user-select: none nodes either. It seems to me that the fix for this bug makes it perhaps easier to fix that bug also.
(Reporter)

Updated

9 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 12

9 years ago
Sorry, but I now looked at the patch and I'm beginning to wonder, why adding a selectionType to setSelected methods?
Apparently, it isn't possible to do the IsSelectable() check in the nsTypedSelection::selectFrames function?
(Assignee)

Comment 13

9 years ago
roc: correct me if i'm talking nonsense!

>Apparently, it isn't possible to do the IsSelectable() check in the
>nsTypedSelection::selectFrames function?

SetSelected recursively sets child frames, and a selectable frame can have a non-selectable child frame. Performing the IsSelectable() test outside of SetSelected, if the test decides that a frame is selectable, SetSelected could then select child frames that are not supposed to be selectable.
(Reporter)

Comment 14

9 years ago
Thanks for the explanation. Makes sense to me.
You need to log in before you can comment on or make changes to this bug.