792420 - Toolbar menu shouldn't allow its text to be selected

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: screenshot

Even though the menu is built in html, we shouldn't be allowing text selection to work on it. See screenshot

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b792420

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 674012 [details] [diff] [review]
b792420

Review of attachment 674012 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/aboutReader.html
@@ +26,5 @@
>      <li><a id="share-button" class="button share-button" href="#"></a></li>
>      <ul class="dropdown">
>        <li><a class="dropdown-toggle button style-button" href="#"></a></li>
>        <li class="dropdown-popup">
> +        <ul id="color-scheme-buttons" class="segmented-button" onmousedown="return false;"></ul>

What's this for? Remove it.

::: mobile/android/chrome/content/aboutReader.js
@@ +545,5 @@
>  
>        let item = doc.createElement("li");
>        let link = doc.createElement("a");
>        link.innerHTML = option.name;
> +      link.style.MozUserSelect = 'none';

We should just put this in the CSS for about:reader:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#445

It looks like we could get rid of this anchor element entirely tbh, and just put the text in the li element, but that's a separate bug I guess (I'm also not sure why this is dynamically generated at all).

Comment 3

[Scott Johnson (:jwir3)]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 674012 [details] [diff] [review]
> What's this for? Remove it.

Ah yes, this was left over from when I was testing something else. I'll remove it.

> We should just put this in the CSS for about:reader:
Hm, so I tried putting the css rule in place. For some odd reason, it doesn't seem to have any effect, though. I tried placing -moz-user-select: none in the rules for the selector .segmented-button > li > a, with and without !important, as well as in a selector with just a, again with and without !important, and neither of them seem to have any effect. As a sanity check, I tried placing border: 1px solid red; in the same selector, and that takes effect as expected. 

Is it possible I might be missing something here? Do I need to do something else to get the rule to translate into aElement.style.MozUserSelect?

> It looks like we could get rid of this anchor element entirely tbh, and just
> put the text in the li element, but that's a separate bug I guess (I'm also
> not sure why this is dynamically generated at all).

Submitted bug 805569 for this.

Comment 4

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #3)
> Is it possible I might be missing something here? Do I need to do something
> else to get the rule to translate into aElement.style.MozUserSelect?

Ok, I found this:

This does not have any affect on actual selection operation. This doesn't have any effect on content loaded as chrome, except in textboxes.

from https://developer.mozilla.org/en-US/docs/CSS/user-select

So, it looks like this CSS property isn't set because this is chrome. Should I just keep it in the JS then, since I'm really only using it as a flag to determine whether or not the selection should be allowed?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to Scott Johnson (:jwir3) from comment #3)
> > Is it possible I might be missing something here? Do I need to do something
> > else to get the rule to translate into aElement.style.MozUserSelect?
> 
> Ok, I found this:
> 
> This does not have any affect on actual selection operation. This doesn't
> have any effect on content loaded as chrome, except in textboxes.
> 
> from https://developer.mozilla.org/en-US/docs/CSS/user-select
> 
> So, it looks like this CSS property isn't set because this is chrome. Should
> I just keep it in the JS then, since I'm really only using it as a flag to
> determine whether or not the selection should be allowed?

Sounds like a good plan.

Comment 6

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f332ed25844e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f332ed25844e

Comment 8

[Cristian Nicolae (:xti)]

This issue doesn't occur anymore on the latest Nightly. Closing bug as verified fixed on:

Firefox for Android
Version: 21.0a1 (2013-01-29)
Device: Galaxy R
OS: Android 2.3.4