765076 - Dynamically attach text selection handle binding

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I attempted this for my patch in bug 695173, but I was having problems getting body.style.MozBinding to work.

We should also assess the performance impact of always attaching this binding vs dynamically attaching/removing it.

Comment 1

[Martijn Wargers (dead)]

Currently, it's possible to disable text selection by setting the root -moz-binding to none: http://people.mozilla.org/~mwargers/tests/selection/xblfun.html
I guess you could change that by adding !important css style, but content can use also the xbl api to access these anonymous nodes, so it would still be possible for content to do weird stuff with this text selection api.

Wouldn't it be better (if that's possible, at least) to add these selection handles inside browser.xul?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.xul

Comment 2

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #1)
> Currently, it's possible to disable text selection by setting the root
> -moz-binding to none:
> http://people.mozilla.org/~mwargers/tests/selection/xblfun.html
> I guess you could change that by adding !important css style, but content
> can use also the xbl api to access these anonymous nodes, so it would still
> be possible for content to do weird stuff with this text selection api.

We are already using anonymous content for the plugin overlays, so I think we're okay with the possibility of websites doing weird thing with the xbl api. However, those overlays use custom pseudo-classes to be bound, so there's less likelihood that the page's CSS will accidentally mess with them.

> Wouldn't it be better (if that's possible, at least) to add these selection
> handles inside browser.xul?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.xul

Unfortunately, if we add the selection handles in browser.xul, we'll need to add more code to handle panning/scrolling/zooming, since the handles currently reposition for free since they're in the same document as the selection itself.

Comment 3

[Martijn Wargers (dead)]

Perhaps some api could be added like there is for the html editor to add anonymous elements?
http://mxr.mozilla.org/mozilla-central/source/editor/idl/nsIHTMLEditor.idl#531

Comment 4

[Boris Zbarsky [:bzbarsky]]

The HTML editor API is a bit of an abomination that causes assertions when actually used (because it's totally violating various layout invariants).

I would be happier if we just made the XBL DOM APIs not work if the principal of the script using them doesn't subsume the binding principal, assuming that's enough for this use case.

Comment 5

[Martijn Wargers (dead)]

I don't think using XBL here is really ideal, but I guess it's enough for this use case.

Attaching an XBL binding to every root element isn't an issue performance wise?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attaching a chrome:// binding should not be a big deal, if it's done via style.

Dynamically attaching after layout has started will cause a full reframe, so is a bit of a problem.

Comment 7

[:Margaret Leibovic]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Attaching a chrome:// binding should not be a big deal, if it's done via
> style.
> 
> Dynamically attaching after layout has started will cause a full reframe, so
> is a bit of a problem.

Oh, in this is the case, it's probably better to just keep the binding attachment in the CSS then. I think we assumed attaching a binding to every root element would be worse than dynamically attaching one when we needed the handles to show up.

Perhaps we can just morph this bug into finding ways to prevent the page from messing with the handles.

Comment 8

[Martijn Wargers (dead)]

The thing is, this stuff isn't normally used, only when a user wants to select something. It just seems overkill to me to always attach a binding for that.
Also, it's applied to every document, it's only needed for the top level browsing context. Is there a CSS selector for that? I don't think it's needed for image documents and video documents, either.
Also, this xbl anonymous content is causing DOMAttrModified events to fire when the select handles positions are changed.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Er, what binging?

Comment 10

[Martijn Wargers (dead)]

The binding added here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/content.css#347
See also bug 695173.

Comment 11

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #8)

> Also, it's applied to every document, it's only needed for the top level
> browsing context.

We actually use whichever binding is attached to the same document as the selection, since that makes positioning the handles easier. However, we could attach this dynamically as needed.

Comment 12

[:Margaret Leibovic]

Not an issue anymore because of bug 774938.

Comment 13

[Martijn Wargers (dead)]

Cool!