1503506 - DRM notification displays unnecessary blank spaces before and after Learn More link

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P5)

Description

[Francesco Lodolo [:flod]]

Attachment: screenshot.png

This is probably less visible in en-US, because there are no characters after the link, but it looks pretty broken in Italian

en-US:
emeNotifications.drmContentDisabled.message = You must enable DRM to play some audio or video on this page. %S
emeNotifications.drmContentDisabled.learnMoreLabel = Learn More

it:
emeNotifications.drmContentDisabled.message = È necessario attivare il DRM per riprodurre i contenuti video o audio presenti in questa pagina. %S.
emeNotifications.drmContentDisabled.learnMoreLabel = Ulteriori informazioni

This results in empty space displayed before and after the link replacing %S.

I can't tell for sure if it's a regression from bug 1431428
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/browser/base/content/browser-media.js#37-45

Comment 1

[Johann Hofmann [:johannh]]

I don't think there needs to be a period after "Learn More"

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> I don't think there needs to be a period after "Learn More"

I'm not saying that there should be one in English, but I have one in Italian, and the layout looks broken, when it shouldn't. I can get rid of the period, but that's taking care of the symptom, and doesn't fix the unnecessarily wide leading space.

BrowserUtils.getLocalizedFragment doesn't seem to be the issue though. It 's used in about:telemetry, and I don't see the same behavior there.

Comment 3

[Johann Hofmann [:johannh]]

Ah, I see, yeah, that makes sense.

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #2)
> BrowserUtils.getLocalizedFragment doesn't seem to be the issue though. It 's
> used in about:telemetry, and I don't see the same behavior there.

And I somehow missed the margins set on the <label> in my own screenshot…

Comment 5

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #0)
> I can't tell for sure if it's a regression from bug 1431428
> https://searchfox.org/mozilla-central/rev/
> fc3d974254660b34638b2af9d5431618b191b233/browser/base/content/browser-media.
> js#37-45

Given this only appears if you willingly disable DRM, or jumped through the relevant hoops to get a default-drm-disabled build, not sure this is high prio, though I could be convinced to make it higher priority if this is indeed a regression and/or affects other panels. Does it?

Comment 6

[Francesco Lodolo [:flod]]

It's not high priority as it is, but I have no way to tell if there other cases. This one is caused by <label> inheriting default CSS, I have no clue if that helps finding them.

Comment 7

[Johann Hofmann [:johannh]]

Yeah, I agree that it's not a high priority, maybe we can find a volunteer to fix it :)

Comment 8

[Manish [:manishkk]]

I can work on this!

Comment 9

[Johann Hofmann [:johannh]]

Ok, thanks!

Comment 10

[Johann Hofmann [:johannh]]

Manish, are you still working on this?

Comment 11

[Manish [:manishkk]]

Hi Johann,

If someone is interested then please assign to them,
else I will work after 2-3 weeks on this.
These days bit busy with work.

Thanks

Comment 12

[Johann Hofmann [:johannh]]

Ok, thanks, unassigning you for now, feel free to pick this up again later.

Comment 13

[Paarmita Bhargava]

Hi,
I am an outreachy applicant, and would like to work on this issue as my second contribution?

Comment 14

[Manish [:manishkk]]

Hi Paramita,
Sure, you can work on this, I think right now no one is working on this.
Submit the patch.
Johann will assign this bug to you.

Comment 15

[Johann Hofmann [:johannh]]

Alright, feel free to give this a shot. You can trigger this by disabling "Play DRM-controlled content" in about:preferences and visiting a site with EME content, such as https://shaka-player-demo.appspot.com/demo/#asset=https://media.axprod.net/TestVectors/v7-MultiDRM-SingleKey/Manifest.mpd;lang=en-US;build=uncompiled

Then you would need to inspect the yellow notification bar with the Browser Toolbox to figure out which CSS rule is adding the extra margin.

Comment 16

[Paarmita Bhargava]

Thanks. I will work on it. But if I need to ask anything where can I contact the mentors?

Comment 17

[Manish [:manishkk]]

IRC channels.
For general help: #Introduction
Firefox: #fx-team

Johann is also available on IRC. his IRC name: [:johannh]

else you can post your questions here and tag person in "Need more information from <Email>" section.

Comment 18

[Johann Hofmann [:johannh]]

Yup, thank you, Manish!

Comment 19

[Paarmita Bhargava]

Hey,
I did the changes and made a patch. Can you please review- https://phabricator.services.mozilla.com/D21594

Comment 20

[Paarmita Bhargava]

Attachment: Bug 1503506 - DRM notification displays unnecessary blank spaces before and after Learn More link

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:paarmita1998, could you have a look please?

Comment 22

[Paarmita Bhargava]

The Patch created is ready to land as the changes are being accepted.

Comment 23

[Johann Hofmann [:johannh]]

Hi Paarmita, did you see my comment in Phabricator? Can you follow-up before landing, please? :)

Comment 24

[Paarmita Bhargava]

Hey Johann, I have checked your comment and it doesn't show the desired behavior on margin: 0px. So it is resolved.

Comment 25

[Johann Hofmann [:johannh]]

(In reply to Paarmita Bhargava from comment #24)

Hey Johann, I have checked your comment and it doesn't show the desired behavior on margin: 0px. So it is resolved.

Ok, thanks for checking that, feel free to set checkin-needed then.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/957cc8c178c0
DRM notification displays unnecessary blank spaces before and after Learn More link r=johannh

Comment 27

[Paarmita Bhargava]

Hey Johann,
Is this issue fixed?

Comment 28

[Francesco Lodolo [:flod]]

(In reply to Paarmita Bhargava from comment #27)

Hey Johann,
Is this issue fixed?

No. It landed on the integration branch, it's not in mozilla-central yet (and consequently not in Nightly).

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/957cc8c178c0