701771 - Context Click menu shows FS option even within FS mode

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Tony Chung [:tchung]]

Attachment: Full screen context click screenshot

Log into full screen video mode, and notice that context click menu still gives the option for full screen.  

See screenshot

Repro:
1) install fennec nightly, 20111111 build
2) visit a site with full screen video (see URL)
3) context click the video sample at the top, and enter full screen
4) once in FS, context click again, and verify the option is still available

Expected:
- FS mode should hide the FS context click option

Comment 1

[:Margaret Leibovic]

Attachment: patch

We'll have to add a new selector when we implement the controls in bug 701785 since we want them to show up regardless of full screen state, but I think this fix makes sense for now since this is the only video context menu item we have right now.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 573933 [details] [diff] [review]
patch

LGTM

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 573933 [details] [diff] [review]
patch

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

I think I'd rather we actually just not cache this Selector. i.e. My original plan was basically to only cache things that I thought we or extensions might need easy access too for more than a few menuitems. That is a fairly vague and nebulous set, but for this one, can we just do something like:

this.add(Strings.browser.GetStringFromName("contextmenu.fullScreen"),
  this.SelectorContext("video:not(:-moz-full-screen)"),
  function(aTarget) {
    aTarget.mozRequestFullScreen();
  }
);

Comment 4

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Comment on attachment 573933 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 573933 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I think I'd rather we actually just not cache this Selector. i.e. My
> original plan was basically to only cache things that I thought we or
> extensions might need easy access too for more than a few menuitems. That is
> a fairly vague and nebulous set, but for this one, can we just do something
> like:
> 
> this.add(Strings.browser.GetStringFromName("contextmenu.fullScreen"),
>   this.SelectorContext("video:not(:-moz-full-screen)"),
>   function(aTarget) {
>     aTarget.mozRequestFullScreen();
>   }
> );

That makes sense - I was just following the model of the other Selectors :)

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/projects/birch/rev/a28cb9b7b7f4

Comment 6

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)