Closed Bug 1269757 Opened 4 years ago Closed 4 years ago

Cleanup panel id and panelId attributes


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



(firefox49 fixed)

Firefox 49
Tracking Status
firefox49 --- fixed


(Reporter: ochameau, Assigned: bigben)




(1 file, 2 obsolete files)

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.
Assignee: nobody → bchabod
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!
Flags: needinfo?(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]:

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.
Attachment #8751288 - Flags: feedback?(janx)
Flags: needinfo?(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")


Alex, do you agree about deriving the "thing-panel" and "thing-panel-header-name" IDs that are hard-coded today (with bugs)?
Attachment #8751288 - Flags: feedback+
(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)?

Attachment #8751288 - Flags: feedback?(janx)
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".
Attachment #8751347 - Flags: review?(janx)
Attachment #8751288 - Attachment is obsolete: true
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.
Attachment #8751347 - Flags: review?(janx) → review+
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.
Flags: needinfo?(bchabod)
Sorry, I'm new to this and messed up my push-to-try.
I cancelled the previous job, here are the real tests
Keywords: checkin-needed
Trying to land this, I'm hitting conflicts:

$ hg import -e
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?
Flags: needinfo?(bchabod)
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).
Priority: -- → P2
Indeed, I rebased on master but there was a recent patch regarding about:debugging in fx-team.
This version should be ready for landing.
Attachment #8753753 - Flags: review+
Attachment #8751347 - Attachment is obsolete: true
Here's the corresponding test!
Bug 1268107 (recently merged into fx-team) was causing the conflict.
Flags: needinfo?(bchabod)
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.