1285802 - Cancel fullscreen does not automatically adjust the screen orientation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[709922234]

Attachment: ScreenRecord_2016-07-10-18-44-20_000.mp4

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

1. enter full screen
2. screen.orientation.lock('landscape')
3. Click the back button to exit fullscreen
4. repeat 1,2,3


Actual results:

Firefox mobile screen orientation is landscape


Expected results:

screen orientation is portrait

Comment 1

[Sorina Florean [:sflorean]]

Hi,
What device did you used when this issue appeared?
What Android version does it have?
Thanks!

Comment 2

[709922234]

Sony Xperia Z5C 
Andriod 6.0.1

Comment 3

[Richard Newman [:rnewman]]

Not a language selection bug.

Comment 4

[Jing-wei Wu [:jwu]]

Attachment: Bug 1285802 - Always add listener when orientation locked.

When remove fullscreenchange listener, mFullScreenListener doesn't
assign to nullptr. But the listener only been added when
mFullScreenListener is null.

Review commit: https://reviewboard.mozilla.org/r/101030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101030/

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

:jwu is still working on this (and waiting for review).

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

I would like to redirect the review to Ben Kelly because I'm not familiar with the DOM code and someone working on this module should review this change.

However reviewboard is complaining:
> Unable to update reviewers as the review request has pending changes (the patch author has a draft)

jwu: Do you still have a draft of an updated version that you need to publish on reviewboard? If you are going to publish the review request then just change the reviewer to "bkelly" before doing that. Thanks!

Comment 7

[Jing-wei Wu [:jwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)

I have changed the reviewer to "bkelly" on reviewboard.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821951 [details]
Bug 1285802 - Always add listener when orientation locked.

Did you press 'publish'? Seems like I'm still set as reviewer.

Comment 9

[Jing-wei Wu [:jwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
Sorry, the change has be published.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I don't know this code either.  William, can you take a look?

It seems this change will make us fire system events more often and its unclear to me if that is ok.

Comment 11

[William Chen [:wchen]]

Comment on attachment 8821951 [details]
Bug 1285802 - Always add listener when orientation locked.

https://reviewboard.mozilla.org/r/101030/#review104460

::: dom/base/ScreenOrientation.cpp:376
(Diff revision 1)
> -  if (aIsFullScreen && !mFullScreenListener) {
> +  if (aIsFullScreen) {
> +    if (!mFullScreenListener) {
> -    mFullScreenListener = new FullScreenEventListener();
> +      mFullScreenListener = new FullScreenEventListener();
> +    }
> +
>      aRv = target->AddSystemEventListener(NS_LITERAL_STRING("fullscreenchange"),

It looks like AddSystemEventListener ignores duplicate attempts to add the same event listener with the same parameters so this should be fine.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Autoland can't land this until all pending issues are marked as resolved in MozReview.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b706ca522fdd
Always add listener when orientation locked. r=wchen

Comment 14

[Carsten Book [:Tomcat]]

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