1331672 - "Exit Full Screen" button on the video fullscreen UI isn't properly vertically aligned

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[(no longer active)]

Attachment: Screenshot

See the screenshot.

Comment 1

[Dão Gottwald [::dao]]

Probably just need to remove the top and bottom margin from #fullscreen-exit-button in browser/themes/shared/fullscreen/warning.inc.css.

Comment 2

[Timothy Pan]

I'd like to work on it. Its my first bug, so give me some time.

Comment 3

[Dão Gottwald [::dao]]

(In reply to Timothy Pan from comment #2)
> I'd like to work on it. Its my first bug, so give me some time.

It's not an urgent issue, so take your time! Let me know if you have questions.

Comment 4

[Srivatsav Gunisetty]

Hey, I could not reproduce it in nightly 54.0. And I cannot even see the #fullscreen-exit-button in the specified path.

Comment 5

[Dão Gottwald [::dao]]

(In reply to Srivatsav Gunisetty from comment #4)
> Hey, I could not reproduce it in nightly 54.0.

What operating system are you using? I think this is more visible on macOS than on other platforms.

> And I cannot even see the
> #fullscreen-exit-button in the specified path.

You would need to add a new rule for that.

Comment 6

[Timothy Pan]

As far as I can tell it looks like the problem is with the rule "html|*.pointerlockfswarning-exit-button", because it does not to be applied at all. As you suggested once I changed the selector back to #fullscreen-exit-button everything worked, all the declarations including setting margins to 0 were applied. 

But since all of the other rules work and use a class selector, I suppose the best way to fix this would be to add the class to the button, instead of using id selector.

Comment 7

[Srivatsav Gunisetty]

Attachment: Screenshot_nightly(54.0)_linux.png

I Think this issue is not present with nightly(54.0) Linux. I am attaching the screenshot.

Comment 8

[Dão Gottwald [::dao]]

(In reply to Timothy Pan from comment #6)
> As far as I can tell it looks like the problem is with the rule
> "html|*.pointerlockfswarning-exit-button", because it does not to be applied
> at all. As you suggested once I changed the selector back to
> #fullscreen-exit-button everything worked, all the declarations including
> setting margins to 0 were applied. 

I'm not sure what the point of this obsolete rule is. Looks like nobody missed it except for the margin issue. I'd review a patch that updates the selector and strips down the rule to just removing the margin.

> But since all of the other rules work and use a class selector, I suppose
> the best way to fix this would be to add the class to the button, instead of
> using id selector.

This is because those elements occur twice in browser.xul. The button exists only once, though, so let's keep using the id.

Comment 9

[Arshad Khan [:arshad]]

hi, can i also work on this bug ? I see Timothy is also trying to work on it.

Comment 10

[Dão Gottwald [::dao]]

Timothy, do you want to keep working on this?

Comment 11

[Timothy Pan]

Yes, I can do it. I've just been a little busy. I just have to read the documentation on how to submit the fix properly.

Comment 12

[Arshad Khan [:arshad]]

ok cool, I will look for another bug!

Comment 13

[Timothy Pan]

Attachment: Fix CSS rule to select by ID and set margins to 0 only.

Comment 14

[Dão Gottwald [::dao]]

Comment on attachment 8837214 [details] [diff] [review]
Fix CSS rule to select by ID and set margins to 0 only.

Looks good. Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3cc6b9b36b3
Change fs warning button css rule to use ID selector and remove margins. r=dao

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Tanvir Rahman]

Attachment: attachment.png

While verifying the fix of this bug with Latest Nightly 55.0a1 (2017-03-29)on Windows 10 (64 bit), I felt that the "Exit Full Screen" button is not properly aligned.   
I have attached a screenshot of the fix that I saw.Is the current fix (attached in the attachment) should be expected as the fix?

[bugday-20170329]

Comment 18

[Gabi Cheta]

I can still reproduce this issue on the latest nightly using Windows 10 x64.