777450 - Long-tap on Flash video selects all text on screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Aaron Train [:aaronmt]]

Currently when HTML5 video is playing (e.g, desktop YouTube), any attempts to enter full-screen via long-tapping on the playing-video will not invoke a context menu but instead select all text on the screen.

Steps to reproduce (finding an a web-app with HTML5 is tough, going to use YouTube here)

i) Visit http://ed.agadak.net/app/, install and launch 'Ed Agadak'
ii) Inside 'Ed Agadak', tap the YouTube link
iii) From YouTube, visit the desktop version via the footer link
iv) Find an HTML5 video from the 'Popular' section such as 'Champion Spotlight: Zyra, Rise of the Thorns'
v) Start the video and try and enter full-screen via long-tap and through a context-menu.

ER: Context-menu
AR: All text on the page is selected

--
Nightly (07/25)
Galaxy Nexus (Android 4.1.1)

Comment 1

[:Margaret Leibovic]

Is this an issue on beta? Trying to judge how time-sensitive a fix is here.

Comment 2

[Aaron Train [:aaronmt]]

16/17

Comment 3

[:Margaret Leibovic]

The source of the text selection problem is that we're not showing the context menu for some reason.

Michael, you've recently been working with our context menus, so would you have time to look into this? Feel free to say no, I just thought it wouldn't hurt to ask :)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Here is a link to a Firefox compatible HTML5 video: http://camendesign.com/code/video_for_everybody/test.html

Long pressing on that video brings up the expected context menu that allows full screen.

I was unable to get an HTML5 video to load from Youtube, using the method above or my own method (see bottom).

However, when I long press on Flash videos, the entire screen highlights so it's possible you were watching a Flash video.

In any case, I would say that it's undesired behavior to have the entire screen highlight when you long press a Flash video (especially considering they look so much like HTML5 videos!). Seeing no errors with HTML5 video long presses, I think this specific bug instance should change into a fix for Flash video long presses. If you feel I am in error, please let me know.

--- 2nd Ineffective Youtube method
0) Using desktop, join HTML5 trial ("Try something new" link @ bottom when signed into a Youtube account; Below "HTML5 Video", click "Try it out"; on the next page, you may need to click "Join the HTML5 Trial").
1) Using desktop, find a video that plays as HTML5, rather than Flash (I used the first non-featured result when searching "IU", https://www.youtube.com/watch?v=0U3UHj-xtHw). To tell the difference between flash and HTML5, you can right-click and the menu will appear differently between the two; notably HTML5's menu does not mention Flash.
2) Using mobile, log into the Youtube desktop site using the same account.
3) Using mobile, find the video from step #1... Does not play HTML5. :(

I also attempted this using the Phony add-on and a user agent of "Desktop Firefox".

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Cleaned up extraneous code in context menus.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01: Cleaned up extraneous code in context menus

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 02: Fixed Flash long press = select all text

There are probably some other elements without context menus that we would want to prevent text selection on when the item is long pressed. Can you think of any that should be added?

Also, we might want to indicate to the user that the long press has been triggered with something like haptic feedback, like when a context menu shows up. Similarly, haptic feedback should probably occur when text selection begins. What do you think?

Comment 8

[Wesley Johnston (:wesj)]

I think we also want to disable selection on media elements (image, video, audio), and maybe input elements where we know selection won't work (select?). I think that's a good start. Can you update or add a part three?

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 03: (Optional) Haptic feedback on text selection and non-context menu long presses

If you want it.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Wesley Johnston (:wesj) from comment #8)
> I think we also want to disable selection on media elements (image, video,
> audio), and maybe input elements where we know selection won't work
> (select?). I think that's a good start. Can you update or add a part three?

The context menus come into play on at least image and video. I'm not sure about audio. I'll look into select as well.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 648179 [details] [diff] [review]
01: Cleaned up extraneous code in context menus

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

Nice cleanup.

::: mobile/android/chrome/content/browser.js
@@ +1388,5 @@
>          element = element.parentNode;
>        }
>  
>        // only send the contextmenu event to content if we are planning to show a context menu (i.e. not on every long tap)
> +      if (!objectIsEmpty(this.menuitems)) {

Tricky. I don't really want the global helper function around though. Two choices, move it somewhere (in NativeWindow.contextMenus is fine we already have some helpers here), or do something tricky like:

if (Object.keys(this.menuitems).length > 0)

That will likely be slower, but is more contained. I can't really decide what I like better.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 648180 [details] [diff] [review]
02: Fixed Flash long press = select all text

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

This looks good. Just want to see it again before it lands.

::: mobile/android/chrome/content/browser.js
@@ +1372,5 @@
> +     */
> +    noActionContext: {
> +      matches: function noActionContext(aElement) {
> +        return (aElement instanceof Ci.nsIDOMHTMLEmbedElement ||
> +                aElement instanceof Ci.nsIDOMHTMLButtonElement);

Normally I'd leave things to a follow up, but I really just want to kill MediaElements here:

aElement instanceof Ci.nsIDOMHTMLMediaElement

I think that will catch images/videos/audio which don't have context menus in webapps. While you're at it, I think we can just move this to a helper function and not do the matches stuff here.

@@ +1407,5 @@
>                               0, null);
>          rootElement.ownerDocument.defaultView.addEventListener("contextmenu", this, false);
>          rootElement.dispatchEvent(event);
> +      } else if (this.noActionContext.matches(rootElement)) {
> +        // Do nothing.

i.e. this can just be:

} else if (SelectionHelper.canSelect(rootElement)) {
  SelectionHandler.startSelection(root...
}

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 648184 [details] [diff] [review]
03: (Optional) Haptic feedback on text selection and non-context menu long presses

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

I'm fine with doing these things. I do wonder if users will be confused when they long tap and nothing happens. I also wonder if they'll be confused if they long tap, we vibrate, and still nothing happens. Maybe UX can help? ian?

mcomella, it would help if you could throw up a build with this in it for ux to try? Then pester them to do it on irc. If you need a people.mozilla.org account hollar, but you can put it up anywhere.

Comment 14

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Tricky. I don't really want the global helper function around though. Two
> choices, move it somewhere (in NativeWindow.contextMenus is fine we already
> have some helpers here), or do something tricky like:
> 
> if (Object.keys(this.menuitems).length > 0)
> 
> That will likely be slower, but is more contained. I can't really decide
> what I like better.

I just asked Dave Herman what's the best way to do this. He said "Don't". Heh. Apparently if we ever start playing with Object.__proto__ stuff we'd get in trouble. I doubt we'll ever do that, but safer way might be just to just hold a boolean. Anyway, I learned something (there's no good way to do this)!

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01a: Cleaned up extraneous code in context menus

Note: Previous patch had r+.

Removed the helper function in favor of a boolean variable. I agree that it's better to encapsulate the code, especially in a lengthy file like browser.js.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #15)

> Removed the helper function in favor of a boolean variable. I agree that
> it's better to encapsulate the code, especially in a lengthy file like
> browser.js.

At least it's a local and not a data member. A boolean data member would make me sad.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Wesley Johnston (:wesj) from comment #13)
> I'm fine with doing these things. I do wonder if users will be confused when
> they long tap and nothing happens. I also wonder if they'll be confused if
> they long tap, we vibrate, and still nothing happens. Maybe UX can help? ian?

I think it's equivalent either way. If you don't have feedback, users will think they missed the element they are trying to long press and try again. If you do have feedback, users will still think they missed the element (or it glitched out to not show a menu). However, providing haptic feedback gives the user an indication of when to stop long pressing, so I prefer that.

> mcomella, it would help if you could throw up a build with this in it for ux
> to try...

Haptic feedback test build: https://dl.dropbox.com/u/23545994/fennec_mcomella_haptic.apk

Haptic feedback is added to "Embed" (generally Flash elements) and "Button" tags with the intent to add it to all tags which do not have a long press context menu. Additionally, it is added to long press text selection. Essentially, we should provide haptic feedback for all long press actions, with or without context menus. Test it out and let me know what you think.

Comment 18

[Ian Barlow (:ibarlow)]

(In reply to Michael Comella (:mcomella) from comment #17)
> (In reply to Wesley Johnston (:wesj) from comment #13)
> > I'm fine with doing these things. I do wonder if users will be confused when
> > they long tap and nothing happens. I also wonder if they'll be confused if
> > they long tap, we vibrate, and still nothing happens. Maybe UX can help? ian?
> 
> I think it's equivalent either way. If you don't have feedback, users will
> think they missed the element they are trying to long press and try again.
> If you do have feedback, users will still think they missed the element (or
> it glitched out to not show a menu). However, providing haptic feedback
> gives the user an indication of when to stop long pressing, so I prefer that.

I disagree. Without any haptic feedback, it is actually clearer that nothing is meant to happen on a long press than it is to have your phone vibrate with no subsequent menu or message appear. You're left wondering, "Did I just copy something? Save it? What did I just do?"

> Essentially, we should provide haptic feedback for all long press actions,
> with or without context menus. Test it out and let me know what you think.

No, I don't like this approach. We should only provide haptic feedback on long press actions that actually do something that actually do something and that a user can see, like open a context menu for example.

Comment 19

[Wesley Johnston (:wesj)]

mcomella ping? You have a new patch for me?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 02a: Fixed Flash long press = select all text

Sorry for the delay, I lost this under a few other patches I was working on.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I realized I wasn't sure the exact functionality where the MediaElement is necessary. I was going to compare my build with Nightly on some apps but the Marketplace doesn't think I'm running Nightly when I try to download apps and thus I cannot do this (I will file a bug if I still see problems tomorrow).

Can you mention some use cases where I can test MediaElement?

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 651100 [details] [diff] [review]
02a: Fixed Flash long press = select all text

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

::: mobile/android/chrome/content/browser.js
@@ +1661,5 @@
>  
>      this._ignoreCollapsedSelection = false;
>    },
>  
> +  /** Returns true if the provided element can be selected in text selection, fales otherwise. */

nit fales = false

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 02b: Fixed Flash long press = select all text

As talked about with wesj in-person, added ImageElement to the check.

As also talked about in-person, moved r+ despite the above change.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01b: Cleaned up extraneous code in context menus

Rebase to trunk. Moved r+.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try since no results are shown here. Also note that both patches were bitrotted. Please make sure that the un-bitrotting was done correctly and re-request checkin if the Try push is green. Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=d8c7b73ca48c

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #25)
> https://tbpl.mozilla.org/?tree=Try&rev=d8c7b73ca48c

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/db27c82f7225
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcdf207aced

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db27c82f7225
https://hg.mozilla.org/mozilla-central/rev/1dcdf207aced

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Ryan VanderMeulen from comment #25)
> Pushed to Try since no results are shown here. Also note that both patches
> were bitrotted. Please make sure that the un-bitrotting was done correctly
> and re-request checkin if the Try push is green. Thanks!
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d8c7b73ca48c

Un-bitrotting looks good. Sorry about that – I thought I updated my repo.

I'll try to get try access for the future.

Comment 29

[Aaron Train [:aaronmt]]

Uplift to firefox-16 on Beta?

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #29)
> Uplift to firefox-16 on Beta?

This would have been easy to uplift a few days ago.

Do we feel this is a common situation? Do we only really need Patch 02b?

Comment 31

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #30)
> (In reply to Aaron Train [:aaronmt] from comment #29)
> > Uplift to firefox-16 on Beta?
> 
> This would have been easy to uplift a few days ago.
> 
> Do we feel this is a common situation? Do we only really need Patch 02b?

ping?

Comment 32

[Cristian Nicolae (:xti)]

I cannot reproduce this issue on the latest m-a and m-c builds (09/18). Closing bug as verified.

--
Device: Galaxy Note
OS: Android 4.0.4