2023913 - The city name heading level doesn't follow the correct heading level order

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P3)

Description

[Natalie Stroud [:nstroud]]

STR:

1. Turn on NVDA
2. Open Nightly
3. Ensure that the weather widget is in the detailed view
4. Using the TAB key, move focus into the page
5. Press H to begin navigating by headings. Continue to press this key until you've reached the 'Widgets' heading followed by the cityName heading right after

Actual:

The heading levels jump from h1 (“Widgets”) to h3 (cityName). NVDA says ‘Widgets heading level 1’ and ‘[city name] heading level 3’

Expected:

With ‘Widgets’ being <h1>, the cityName heading should be an <h2>. NVDA should say ‘[city name] heading level 2]’

Comment 1

[Reem Hamoui]

To help Mozilla out with this bug, here's the steps:

1. Comment here on the bug that you want to volunteer to help, and under the text field, check the "Request information from" and write "[e-mail]". This will tell the mentor that you're working on the next steps.

2. Download and build the Firefox source code: https://firefox-source-docs.mozilla.org/setup/index.html

   • If you have any problems, please ask on the bug or reach out to the team.
   • You can also read the Firefox Contributors' Quick Reference, which has answers to most development questions: https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

3. Start working on this bug. Follow the steps the reporter of this bug has put in to replicate the issue.

   • If you have any problems with this bug, please comment on this bug and set the needinfo flag for me.

4. Run and test your newtab changes:

   • Open 2 terminal windows, both in the firefox directory:
     • Window #1: Run ./mach newtab watch (this will watch for code changes)
     • Window #2: Run ./mach run (this will launch Firefox)
   • To reload the page with fresh code changes:
     • Mac: Press Cmd+Opt+R
     • Windows/Linux: Press Ctrl+Shift+R
   • Check your changes for adherence to our style guidelines by using ./mach lint

5. Commit your changes with a proper commit message:

   • Before you submit your changes, make sure to run ./mach newtab bundle and ./mach lint --fix. These commands will bundle up your changes and make sure there are no linting errors left behind.
   • Use the following format for your commit message:
     Bug XXXXXX - Text explaining your fix. r=#home-newtab-reviewers
     • Replace XXXXXX with the bug number (found at the end of this bug's URL)
     • Example: Bug 1234567 - Fix broken link in Top Sites menu. r=#home-newtab-reviewers
   • Commit your changes with: git commit -am "Bug XXXXXX - Your description here. r=#home-newtab-reviewers"
   • Important: Phabricator only needs one commit per bug. If you need to make additional changes after committing, use git commit --amend --no-edit to update your existing commit instead of creating a new one.

6. Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.

   • Getting your code reviewed: https://firefox-source-docs.mozilla.org/setup/contributing_code.html#getting-your-code-reviewed
   • This is when the bug will be assigned to you.

7. After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!

8. ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 2

[Oluwatobi]

Can an Outreachy applicant work on this bug? If so, please assign it to me.

Comment 3

[Reem Hamoui]

Hey there, yes of course! I've assigned it to you. Let me know if you need any help with this.

Comment 4

[Oluwatobi]

I observed that the FocusTimer Widget’s timer notification title also jumps from h1 (“Widgets”) to h3 (Timer). NVDA says ‘Widgets heading level 1’ and ‘Timer heading level 3’. Should I fix this as part of this bug patch, or would you prefer it as a separate bug?

Comment 5

[Reem Hamoui]

If it's an easy enough change then I don't see the harm in fixing it as part of this bug :) thanks for catching that!

Comment 6

[Oluwatobi]

Attachment: Bug 2023913 - Fix incorrect heading-level ordering in the weather forecast and focus timer widgets. r=#home-newtab-reviewers,reemhamz,nsharpley

Comment 7

[Oluwatobi]

I see a red icon next to “home-newtab-reviewers” in the “Phabricator Revisions” section above labeled “Blocking Review.” Does adding r=#home-newtab-reviewers to the commit message, as described above, prevent my patch from being reviewed?

Additionally, while following the helpful steps provided above, I found that the ./mach newtab watch and ./mach newtab bundle commands did not work. Upon reviewing the mach_commands.py file, I noticed a comment recommending the use of ./mach npm run bundle --prefix=browser/extensions/newtab.

It looks like the commands might have changed. Here are the commands that worked for me:

• Terminal #1: ./mach npm run watchmc --prefix=browser/extensions/newtab
• Terminal #2: ./mach build faster && ./mach run
• Bundle changes: ./mach npm run bundle --prefix=browser/extensions/newtab

Comment 8

[Reem Hamoui]

Hm odd, those should work as intended as that's what the majority of the New Tab team uses.
Curious, did you happent to run ./mach newtab install when setting things up? I realized I haven't written it in the steps above so that may be the reason why those commands are not working.
Try running that command and see if ./mach newtab watch and ./mach newtab bundle work after that.

Comment 9

[Reem Hamoui]

I see a red icon next to “home-newtab-reviewers” in the “Phabricator Revisions” section above labeled “Blocking Review.” Does adding r=#home-newtab-reviewers to the commit message, as described above, prevent my patch from being reviewed?

The red icon you see means that the patch can't land without proper review and approval from the #home-newtab-reviewers group. This is a preventative measure that basically helps to make sure that any code changed in the /newtab/ directory gets reviewed by its stewards. We have plenty of these groups across the Firefox codebase to make sure all code is reviewed by the appropriate parties.

In this case, because I'm part of the New Tab team, my review helps determine legibility of the code changes, and updates the blocking #home-newtab-reviewers status based on my assessment of the patch.

Comment 10

[Oluwatobi]

Since we’re updating the FocusTimer <h2> font size to match WeatherForecast, should we make their font weights the same too?

Here are the current font-weight values:

• FocusTimer: var(--font-weight-heading, var(--heading-font-weight)) (computes to 600).
• WeatherForecast: var(--font-weight) (computes to normal).

If we want FocusTimer to use the same heading weight as WeatherForecast, I’ll update the h2 selector in _FocusTimer.scss like this:

h2 {
  font-size: var(--font-size-root);
- // @backward-compat { version 150 } D281466/D283723 renamed design tokens; remove once Firefox 150 is on Release.
- font-weight: var(--font-weight-heading, var(--heading-font-weight));
+ font-weight: var(--font-weight);
  margin-block: 0;
}

Comment 11

[Oluwatobi]

(In reply to Reem Hamoui from comment #8)

Curious, did you happent to run ./mach newtab install when setting things up? I realized I haven't written it in the steps above so that may be the reason why those commands are not working.

I ran ./mach newtab install, but it still isn’t working. You can see the error in this GitHub gist: https://gist.github.com/oluwatobiss/3b446a9821e4a7d5a3f9b1f51b60edea.

When I run ./mach npm install --prefix=browser/extensions/newtab, I get an error indicating a conflict between my system’s Node version and the Firefox newtab package. (https://gist.github.com/oluwatobiss/85791291b30796ed52b8088e2fde749d)

So, I downgraded my Node.js version using NVM, but that didn’t fix the problem. Instead, I get a different error indicating that the newtab extension requires Node v16.19.1, but @testing-library/react requires Node v18 or higher. (https://gist.github.com/oluwatobiss/25761043b3099f77404e3000e1421729)

Could there be a conflict in the project’s dependency tree?

Comment 12

[Reem Hamoui]

Since we’re updating the FocusTimer <h2> font size to match WeatherForecast, should we make their font weights the same too?

Sure, since it's an easy change I don't see the harm in it, and it means we have one less @backward-compat to worry about. thanks for calling it out!

Comment 13

[Reem Hamoui]

Could there be a conflict in the project’s dependency tree?

Ah, I think i may know what may be going on here. We had some React 19 updates land on New Tab recently and that has caused some disruption in our packages. Try running ./mach npm install --legacy-peer-deps --prefix=browser/extensions/newtab and then run ./mach newtab install -- does that fix the issue you're seeing?

Comment 14

[Oluwatobi]

(In reply to Reem Hamoui from comment #13)

Try running ./mach npm install --legacy-peer-deps --prefix=browser/extensions/newtab and then run ./mach newtab install -- does that fix the issue you're seeing?

Running npm install --legacy-peer-deps inside browser/extensions/newtab works.

But when I go back to the root directory (/c/mozilla-source/firefox) and run ./mach npm install --legacy-peer-deps --prefix=browser/extensions/newtab or ./mach newtab install, I still get an error.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/45b010953762
https://hg.mozilla.org/integration/autoland/rev/c149b013493a
Fix incorrect heading-level ordering in the weather forecast and focus timer widgets. r=reemhamz,home-newtab-reviewers

Comment 16

[Cristian Tuns]

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

Comment 17

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

The patch landed in nightly and beta is affected.
:stsofela, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Oluwatobi]

Hi :reemhamz, is there any action required from me regarding comment 17 (BugBot’s uplift comment)?

Comment 19

[Reem Hamoui]

All good, I've updated it :) Thank you again for your contribution, it seems your fix has successfully landed and should be available in Firefox Nightly, and will be released in Firefox v151 when that releases \o/

Comment 20

[Oluwatobi]

(In reply to Reem Hamoui from comment #19)

All good, I've updated it :) Thank you again for your contribution, it seems your fix has successfully landed and should be available in Firefox Nightly, and will be released in Firefox v151 when that releases \o/

Fantastic! Thanks so much for your support.