306262 - [FIX]Unable to select in chrome-docshell documents

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Selection is disabled in documents that are in a chrome docshell; the culprit is
the code in PresShell::Init that checks the docshell type before calling
SetDisplaySelection().

This code was initially checked in with the following checkin comment by mjudge:

  3.124 <[e-mail]> 1999-04-29 13:24
  m5 BUGS Choffman approved. simon reviewed. removing possible memory leak and
  getting anchor and focus frame to use the proper range instead of addreffing a
  node each! tested this afternoon

Simon, I realize this is a little silly, but do you know why this check for
docshell type was added?

I propose to remove this check, since it causes issues like that described in
bug 306108.  As Neil pointed out, that causes checkbox label text to be
selectable, since they use an HTML div to show the label (why not a <xul:label>
with child nodes?).  Of course this means that remote XUL is just broken. 
Wouldn't it make more sense to use our -moz-user-select stuff in whatever way is
needed here to prevent selection of labels instead?

Comment 1

[Brendan Eich [:brendan]]

What a crock that mjudge comment is.

Aren't we likely to want this for 1.8 given bug 306108?

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

It doesn't help bug 306108 because Chatzilla needs to be compatible with all
previous stable Firefox and Mozilla Suite releases (including Mozilla 1.0, 1.4,
1.7 and Firefox 1.0).

This could help some other people trying to use our platform, but I doubt we
should block 1.8 on this unless the XPFE/toolkit folks come up with a very quick
fix for the selection-in-labels thing.  The actual core change to treat the two
types of docshells identically is trivial, of course.

Comment 3

[James Ross]

The advantage to getting this into 1.8 (from ChatZilla's POV) is that the next
ChatZilla release (whenever it is) could pick out 1.8 (and later) versions as
suitable to not use type="content". The fact Firefox 1.5 is going to be based on
1.8 makes it a very large audience, and if ChatZilla has only the choice of no
selections or running into the wrappers, I think we'd just end up not support it
(adding a chrome.manifest is out of the question, the auto-generation in FF is
not smart enough to handle the wrappers flag, and the wrappers would infest a
large amount of our code).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  If getting this into 1.8 would help Chatzilla then we should totally do
that.

Neil, mconnor, would just adding style="-moz-user-select: none" to the label
binding help?  It seems to work over here...

Also what other bindings would need similar style?  Description, I assume? 
Anything else?

Comment 5

[Simon Fraser [no longer active]]

Sorry, I don't recall what mjudge was up to there. And good check-in comments
were not his strong point.

Comment 6

[Asa Dotzler [:asa]]

peter or sicking, can you help out here?

Comment 7

[[e-mail]]

(In reply to comment #4)
>Neil, mconnor, would just adding style="-moz-user-select: none" to the label
>binding help?  It seems to work over here...
Sounds good to me... I assume you mean CSS style, rather than binding.

>Also what other bindings would need similar style?  Description, I assume? 
>Anything else?
Not that I know of.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Mailnews seems to use some sort of weird binding for <description
selectable="true"> that implements selection by hand?  I can't quite tell,
because I haven't been able to actually locate this <description
selectable="true"> in our UI.  Scott, bienvenu, do you know where I can find it?
I'd like to test to make sure my patch doesn't break it.

Comment 9

[David :Bienvenu]

mail/base/content/msgHdrViewOverlay.xul ? and the equiv SM file?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The idea here is to enable selection in all documents and explicitly disable it
for the two XUL elements for which we create block frames.  I've tested that
the mailnews "description" (which is not really a description at all) still
works with this patch.	The patch fixes behavior no the testcase in the URL
field of this bug too -- none of the text is selectable with this.

We may want to consider only disallowing selection for some labels and
descriptions (eg if there is a particular attribute set, allow selection) or
something... let me know.

Comment 11

[[e-mail]]

Comment on attachment 194356 [details] [diff] [review]
Proposed patch

Sorry for the delay, I was out yesterday and today my PC crashed and I was only
reminded by majken's bugmail.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194356 [details] [diff] [review]
Proposed patch

I guess we want to take this on branch, based on the blocking flag.

The only risk is that something that shouldn't really be selectable will end up
selectable, but that will be easy and safe to fix if it comes up.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 194356 [details] [diff] [review]
Proposed patch

let's hold off until beta2. minused for beta1, plussed for beta2

Comment 15

[TimoT]

This caused/exposed bug 307256

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.