1645789 - "Basic Page Style" entry in the View > Page Style menu doesn't work reliably

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P3)

Description

[Schalk Neethling [:espressive]]

Attachment: Screen Recording 2020-06-15 at 13.52.49.mp4

I have the following stylesheets defined in the head section of the HTML document:

<link rel="stylesheet" href="css/main.css" media="screen" />
<link rel="alternate stylesheet" href="css/high-contrast.css" media="screen"
        title="High Contast" />
<link rel="alternate stylesheet" href="css/dyslexia.css" media="screen"
        title="Open-Dyslexic" />

When I load the page in Firefox, the View > Page Style menu is populated by these alternate stylesheets. Also, switching to any of the alternates does update the page as expected.

When switching back to Basic Page Style however, the original page style is not reflected. It seems that whatever the last alternate selected stylesheet was, is assigned to Basic Page Style. The only way to get back to the original styling is to reload the page.

The attache video demonstrates the problem described.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Live repro: https://crisal.io/tmp/alt-sheet.html

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This was regressed by bug 1447009 effectively.

Comment 3

[Alice0775 White]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

This was regressed by bug 1447009 effectively.

Hmm, I got a different window:
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8ebcde6a7778fdd450b948f59992f7295c6b9ff7&tochange=ecc9cefef68de7ba316fb8d63848e907f649a801

Comment 4

[Alice0775 White]

Attachment: red.css

Comment 5

[Alice0775 White]

Attachment: black.css

Comment 6

[Alice0775 White]

Attachment: white.css

Comment 7

[Alice0775 White]

Attachment: Bug1645789.html

Comment 8

[Alice0775 White]

Attachment: Bug1645789.html

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Alice0775 White from comment #3)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

This was regressed by bug 1447009 effectively.

Hmm, I got a different window:
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8ebcde6a7778fdd450b948f59992f7295c6b9ff7&tochange=ecc9cefef68de7ba316fb8d63848e907f649a801

Ah, indeed. Seems like the code was rewritten after my change... Anyhow the fix is trivial. Thanks alice!

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1645789 - Fix "Default Page Style" submenu to do the right thing. r=Gijs

We compare the value passed through with the title, and StyleSheet.title
returns null for empty titles, so we never consider an empty title to be
in the document, which is bad.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1645789 - Also fix the case when we're resetting the styles and the document has no default style. r=Gijs

Depends on D79700

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2ee72003dc9
Fix "Default Page Style" submenu to do the right thing. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/bf23c417bc98
Also fix the case when we're resetting the styles and the document has no default style. r=Gijs

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c2ee72003dc9
https://hg.mozilla.org/mozilla-central/rev/bf23c417bc98

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to ESR78?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

This is a very old regression, but on the other hand it might be nice to fix it for ESR. Gijs, thoughts?

Comment 16

[:Gijs (he/him)]

Sure, taking this on esr seems fine

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9156741 [details]
Bug 1645789 - Fix "Default Page Style" submenu to do the right thing. r=Gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Broken UI (though an uncommon one).
• User impact if declined: Page style menu doesn't work as it should.
• Fix Landed on Version: 79
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straight forward patch, lots of new tests.
• String or UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9156741 [details]
Bug 1645789 - Fix "Default Page Style" submenu to do the right thing. r=Gijs

Fixes problems with the Default Page Style menu. New regression for users updating from ESR68 to ESR78. Approved for 78.1esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/bef6e89f6030
https://hg.mozilla.org/releases/mozilla-esr78/rev/d469bfda63b4

Comment 20

[Dante]

Not sure this is relevant or related ... but View > Page Style menu seems to ignore alternate stylesheets when displaying a local HTML file.

See discussion here:
https://support.mozilla.org/en-US/questions/1293681

For example, this simple HTML file saved on Win10 as %TEMP%\alternate_stylesheets.html

<!DOCTYPE html>
<!-- See for example: https://www.w3.org/Style/Examples/007/alternatives -->
<html>
<head>
<link rel="stylesheet"
href="https://www.w3.org/Style/CSS/w3c-2010/main.css" title=Main>
<link rel="alternate stylesheet"
href="http://www.w3.org/StyleSheets/Core/Steely" title=Steely>
<link rel="alternate stylesheet"
href="http://dbaron.org/style/forest" title="Forest (by David Baron)">
</head>
<body>
<h1>Hello, World!</h1>
</body>
</html>

Comment 21

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I have reproduced the issue on Firefox Nightly 79.0a1 (2020-06-15) under macOS 10.15.5 using the testcases provided by Emilio and Alice.

The issue is fixed on Firefox Nightly 80.0a1 (2020-07-06), Firefox 79.0b4 and Firefox 78.1.0esr (treeherder build). Tests were performed on macOS 10.15.5, Windows 10 and Ubuntu 20.04.