1832855 - Fix up hover styles for text links throughout onboarding & spotlights

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Emily McMinn :emcminn]

Some recent changes to common.css and global-shared.css mean that text-links on about:welcome and in spotlights are no longer inheriting hover styles. Currently, newtab links have:

-underline by default
-no underline on hover
-colour change on active

We should add some rules to aboutwelcome.scss to handle that.

Comment 1

[Punam Dahiya [:pdahiya]]

NI @emcminn to attach screenshots of current behavior in Nightly Fx115

Comment 2

[Emily McMinn :emcminn]

Attachment: Firefox view spotlight

Comment 3

[Emily McMinn :emcminn]

Attachment: center template spotlight

Comment 4

[Emily McMinn :emcminn]

Attachment: Sign in link

Comment 5

[Emily McMinn :emcminn]

Attachment: Newtab hover behaviour

We probably want to, at minimum, match the underline behaviour seen on text links on newtab:

Comment 6

[Negin]

Small update - synced up with Gabrielle who will be syncing up with Jules since design systems wanted to apply some changes to these links regardless of this bug.

And somewhat unrelated to this bug, I was curious why the sign in link is a button rather than an anchor tag? Do you happen to have any context into this decision Emily? My understanding was that it's more semantic and accessible to use an a tag when navigating to a new page, but I might be missing some context into this (or may be wrong altogether!)

Comment 7

[Shane Hughes [:aminomancer]]

(In reply to Negin from comment #6)

Small update - synced up with Gabrielle who will be syncing up with Jules since design systems wanted to apply some changes to these links regardless of this bug.

And somewhat unrelated to this bug, I was curious why the sign in link is a button rather than an anchor tag? Do you happen to have any context into this decision Emily? My understanding was that it's more semantic and accessible to use an a tag when navigating to a new page, but I might be missing some context into this (or may be wrong altogether!)

The CTA's behavior has to be dynamic. In this case the OPEN_URL action opens a new page, but it can be configured to use any action

Comment 8

[Negin]

Attachment: Bug 1832855 - Fix up hover styles for text links throughout onboarding & spotlights

Comment 9

[Emily McMinn :emcminn]

(In reply to Shane Hughes [:aminomancer] from comment #7)

(In reply to Negin from comment #6)

Small update - synced up with Gabrielle who will be syncing up with Jules since design systems wanted to apply some changes to these links regardless of this bug.

And somewhat unrelated to this bug, I was curious why the sign in link is a button rather than an anchor tag? Do you happen to have any context into this decision Emily? My understanding was that it's more semantic and accessible to use an a tag when navigating to a new page, but I might be missing some context into this (or may be wrong altogether!)

The CTA's behavior has to be dynamic. In this case the OPEN_URL action opens a new page, but it can be configured to use any action

We also use a button element to help manage keyboard interaction for a11y :)

Comment 10

[Negin]

Ah gotcha, thanks for the context Shane & Emily!

Comment 11

[Negin]

Scope of this ticket is to reintroduce the default link styling for onboarding and spotlight modals (underline by default, on hover underline disappears)

Comment 12

[Negin]

[Tracking Requested - why for this release]:
This patch is required to fix the link styles for onboarding and spotlight modals. Currently, the sign in button and secondary links have no hover state/do not seem intractable.

Comment 13

[Negin]

To test the fix:

For onboarding:
Visit about:welcome and notice the normal (underline) and hover states (no underline) for sign in link in the top right

For spotlight:
In about:config set browser.newtabpage.activity-stream.asrouter.devtoolsEnabled to true, open a new tab and click the wrench icon. Under messages in ASRouter display the FX_100_UPGRADE message, notice the normal and hover states

To test additional_cta styling:
in about:config set browser.aboutwelcome.screens from the following gist

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39f46465214c
Fix up hover styles for text links throughout onboarding & spotlights r=omc-reviewers,emcminn

Comment 15

[Negin]

Comment on attachment 9334205 [details]
Bug 1832855 - Fix up hover styles for text links throughout onboarding & spotlights

Beta/Release Uplift Approval Request

• User impact if declined: Onboarding and Spotlight links (specifically sign in) do not look interact-able as a result of missing styling and hover states
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Can be found in phabricator test plan or above comment^
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Styling changes only affecting links and their hover states for onboarding and spotlight modals
• String changes made/needed: No
• Is Android affected?: No

Comment 16

[Shane Hughes [:aminomancer]]

Did we previously have default (underline) -> hover (no underline)?
That sounds OK but doesn't seem like the previous behavior. Actually, I'm pretty sure it was the opposite:
default (no underline) -> hover (underline)
Making links underlined by default would be more consistent with the rest of the browser, but I thought we were just trying to revert the changes from bug 1821788 until a11y review?

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/39f46465214c

Comment 18

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this issue using the latest Firefox Nightly 115.0a1 (Build ID: 20230521210743) on Windows 10 x64, macOS 11.7.1, and Ubuntu 22.04 x64.

• After following the testing steps from this Test Plan, I confirm the following:
  The “Sign in” link from the first screen of the “about:welcome” page, the “Not now” link from the “FX_100_UPGRADE” spotlight, and the “test additional button” link from the Gratitude screen are underlined as default and are not underlined when hovering them.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Shane Hughes [:aminomancer] from comment #16)

Did we previously have default (underline) -> hover (no underline)?
That sounds OK but doesn't seem like the previous behavior. Actually, I'm pretty sure it was the opposite:
default (no underline) -> hover (underline)
Making links underlined by default would be more consistent with the rest of the browser, but I thought we were just trying to revert the changes from bug 1821788 until a11y review?

Can we make sure we have the intended behaviour before uplifting to beta? Thanks!

Comment 20

[Shane Hughes [:aminomancer]]

Sorry for the ambiguity in my previous post, didn't have time to check it til now. I just tested on release 113.

On about:welcome, I see:

default: no underline
hover: underline
active: no underline
focus-visible: no style (i.e., doesn't underline but doesn't override the hover style)

As far as I can tell, the links on about:newtab are true links. I don't see any text-link styles on about:newtab. It never imported common-shared.css or global-shared.css (the files changed in the regression), so I think it's just not within the scope of this bug. But just for perspective, their styles were inconsistent with about:welcome in 113. On the actual newtab page proper (not the devtools), I see

default: no underline
hover: underline
active: underline
focus-visible: underline

The about:welcome style above seems to be consistent with common-shared.css prior to bug 1821788. These were the styles in common-shared.css:

.text-link {
  text-decoration: none;
}

.text-link:hover {
  text-decoration: underline;
}

.text-link:hover:active {
  text-decoration: none;
}

Per bug 1821788, the text-decoration rules were removed, and a text-decoration rule was added to global-shared.css that underlines all text-links (also all anchor elements, but keep in mind anchor elements are underlined by UA stylesheets already, so this would only be relevant if a less specific author sheet overrode the UA styles, which is unlikely)

.text-link {
  text-decoration: underline;
}

But because we never imported global-shared.css, but we did import common-shared.css, we didn't get the new rule above, but we did lose the text-decoration rules above that. Also, we never distinguished the secondary top link CTA's states by color, since it's always black/white. So instead of looking like about:preferences (where all links are currently underlined in every state, with only the color distinguishing them), it turned out to be non-underlined and the same color in every state.

So if we want to revert bug 1821788, we'd need styles mirroring the common-shared.css styles above. It seems like we might have the opposite now. If we want to go our own way with these styles that's fine too - idk if this is necessarily a critical issue, but we should just make sure whatever we're doing, it's documented and intentional.

Comment 21

[Shane Hughes [:aminomancer]]

We filed a bug to make a more deliberate decision about hover styles. In the meantime, this is good to go for 114 uplift.

Comment 22

[Pascal Chevrel:pascalc (PTO until April 26)]

The patch does not graft cleanly to the beta branch, yet the uplift request does not mention other uplift needed. I see that browser/components/newtab/content-src/aboutwelcome/aboutwelcome.scss got multiple updates in the 105 nightly cycle, are there other bugs that would need to be uplifted? Please provide a rebased patch for beta, thanks!

Comment 23

[Negin]

Hi Pascal, sorry for the delay! I created a new patch with the resolved beta conflicts and assigned you as a reviewer with the same bug id but beta in the description. Let me know if this is okay and if there's any issues with the patch.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9334205 [details]
Bug 1832855 - Fix up hover styles for text links throughout onboarding & spotlights

Approved D178806 for beta 8, thanks.

Comment 25

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/aef6eaa050ab

Comment 26

[Negin]

Attachment: Bug 1832855 - Fix up hover styles for text links throughout onboarding & spotlights

Comment 27

[Negin]

Attachment: Bug 1832855 - Fix up hover styles for text links throughout onboarding & spotlights beta r=pascalc a=pascalc

Comment 28

[Pascal Chevrel:pascalc (PTO until April 26)]

Backed out for causing newtab failures on beta https://treeherder.mozilla.org/jobs?repo=mozilla-beta&selectedTaskRun=WY_7oFAkQp6fn05bj0C1ug.0&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Copt%2Cnode%2Ctests%2Csource-test-node-newtab-unit-tests%2Cnewtab&revision=18f79b69b59438d998569d811b6f77b0764b3c23
https://hg.mozilla.org/releases/mozilla-beta/rev/5f16ffade66e

Comment 29

[Negin]

Sorry about this! I didn't catch that beta is still using sasslint rather than stylelint (hence why it passes on Nightly but not Beta because the rule does not exist in stylelint), I'll update the patch so it conforms to the sasslint ruleset.

Comment 30

[Negin]

Hi Pascal, apologies for the delay. The beta patch should be ready to land in beta now and is correctly passing sasslint. Let me know if there's any problems or anything needed on my end.

Comment 31

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/06950c28b7b2

Comment 32

[Alice Varro [avarro], Ecosystem QA]

I have verified this issue using the latest Firefox Release 114.0.1 (Build ID: 20230608214645) on Windows 10 x64, macOS 13.2, and Linux Mint 20.2 by following the steps provided in this Test plan, and I can confirm the following:

• The “Sign In” hyperlink from the first screen of the “about:welcome” page, the “Not now” link from the “FX_100_UPGRADE” spotlight in the "asrouter" page, and the “test additional button” link from the "Gratitude" screen are displayed as underlined by default and are not underlined when hovering them.