1269757 - Cleanup panel id and panelId attributes

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

Bug 1266128 renamed tabs to panel and we end up have id *and* panelId on panel objects, which is surely misleading.
`id` is for the id used in the about:debugging urls, like "addons" in "about:debugging#addons".
`panelId` is used for accessibility and refer to the DOM element id of the panel.

Comment 1

[Benoit Chabod [:bigben]]

Attachment: Patch proposal to refactor panelId and id attributes in about:debugging

Alexandre and Jan, here's a patch I wrote to fix that ambiguity.
I decided to rename panelId to id, and to rename the old id to "hash", as it is indeed the location.hash anchor label.
The variables have been changed accordingly in aboutdebugging.js, panel-menu-entry.js and panel-menu.js.
I also fixed a small detail in tabs/panel.js, where the panel id was hardcoded instead of being read from the parameters.
Tell me what you think of it!

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8751288 [details] [diff] [review]
Patch proposal to refactor panelId and id attributes in about:debugging

Review of attachment 8751288 [details] [diff] [review]:
-----------------------------------------------------------------

Let's see if janx things the same.

::: devtools/client/aboutdebugging/components/panel-menu-entry.js
@@ +10,5 @@
>  module.exports = createClass({
>    displayName: "PanelMenuEntry",
>  
>    onClick() {
> +    this.props.selectPanel(this.props.hash);

Same thing here. We could make it so that selectPanel expect the panel id.
It would make it slightly more complex itself, but we would use panel id all over the codebase and keep hash only used within aboutdebugging.js.

::: devtools/client/aboutdebugging/components/panel-menu.js
@@ +11,5 @@
>  module.exports = createClass({
>    displayName: "PanelMenu",
>  
>    render() {
> +    let { panels, selectedPanelHash, selectPanel } = this.props;

Instead of passing selectedPanelHash (and keep hash something contained to aboutdebugging.js) we could pass selectedPanel here.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8751288 [details] [diff] [review]
Patch proposal to refactor panelId and id attributes in about:debugging

Review of attachment 8751288 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for this patch Benoit!

It looks good, but I'd prefer if we could simplify the panel-representation by having only one such parameter, preventing inconsistency errors like we currently have in [0] and [1].

I suggest the following conventions:

- Menu entry <div aria-controls="<ID>-panel" />
- Panel container <div id="<ID>-panel" aria-labelledby="<ID>-header" />
- Panel header <h1 id="<ID>-header" />

(where <ID> is the remaining single parameter `id`, e.g. "addons", "tabs", or "workers")

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/addons/panel.js#118-121
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/workers/panel.js#110-113

Alex, do you agree about deriving the "thing-panel" and "thing-panel-header-name" IDs that are hard-coded today (with bugs)?

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Jan Keromnes [:janx] from comment #3)
> Alex, do you agree about deriving the "thing-panel" and
> "thing-panel-header-name" IDs that are hard-coded today (with bugs)?

Yes

Comment 5

[Benoit Chabod [:bigben]]

Attachment: Second patch to clean up panelId/id confusion in about:debugging.

Thanks for your feedback, here's a second version of the patch!
I've now removed the hash/old id attribute and there's only one parameter left.
Corresponding DOM elements ids are derived using rules such as +"-panel" or +"-header".

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8751347 [details] [diff] [review]
Second patch to clean up panelId/id confusion in about:debugging.

Review of attachment 8751347 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me, thanks a lot!

Please send it to try, using something like:

> git push-to-try -r HEAD -t ../mozilla-central -b do -p linux64,macosx64,win64 -u reftest,mochitests -t none

to make sure your changes work across all platforms (they should), and paste the results link in this bug.

Note: You need a separate mercurial `hg clone` at ../mozilla-central for that to work. It's a bit of a hassle, but we're working on better tools.

Comment 7

[Jan Keromnes [:janx]]

And when you're confident about the try results, please add the `checkin-needed` keyword to this bug, signalling the sheriffs that this patch is ready to be merged into mozilla-central.

Comment 8

[Benoit Chabod [:bigben]]

Patch sent to try!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7b0fc2da703

Comment 9

[Benoit Chabod [:bigben]]

Sorry, I'm new to this and messed up my push-to-try.
I cancelled the previous job, here are the real tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e0b237282c2

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Trying to land this, I'm hitting conflicts:

$ hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8751347
applying https://bugzilla.mozilla.org/attachment.cgi?id=8751347
patching file devtools/client/aboutdebugging/components/aboutdebugging.js
Hunk #2 FAILED at 59
Hunk #3 FAILED at 86
2 out of 3 hunks FAILED -- saving rejects to file devtools/client/aboutdebugging/components/aboutdebugging.js.rej
patching file devtools/client/aboutdebugging/components/panel-menu-entry.js
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/aboutdebugging/components/panel-menu-entry.js.rej
abort: patch failed to apply


Can we get a patch rebased onto an up to date tree?

Comment 11

[Jan Keromnes [:janx]]

Looks like there was a last minute conflict after all. Please rebase and reupload the patch, and carry over my r+.

Also, please send a new try just in case, but you don't have to wait for results before asking for "checkin-needed" again (I expect the rebase will be small, so the previous try push should still have authority).

Comment 12

[Benoit Chabod [:bigben]]

Attachment: Rebased patch to clean up panelId/id confusion in about:debugging.

Indeed, I rebased on master but there was a recent patch regarding about:debugging in fx-team.
This version should be ready for landing.

Comment 13

[Benoit Chabod [:bigben]]

Here's the corresponding test!
Bug 1268107 (recently merged into fx-team) was causing the conflict.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c75cbb0f162

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4217fe065453

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4217fe065453