1895490 - Remove inline event handlers from all tabs menu

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task)

Description

[:Gijs (out for now; he/him)]

https://searchfox.org/mozilla-central/source/browser/base/content/browser-allTabsMenu.inc.xhtml has 3 inline event handlers that we should refactor into "normal" JS so it's not inline in the markup (cf bug 1890547).

Comment 1

[Leeya]

Attachment: WIP: Bug 1895490 - Remove inline event handlers from all tabs menu. r?Gijs!

Comment 2

[Leeya]

Hi Gijs!

I have two clarifying questions:

1. The 3 inline event handlers to remove, don't include line 10: gTabsPanel.searchTabs(), right?

2. I was initially concerned about whether I'd have to move the behavior elsewhere, but from what I can understand, that's already being done in browser-allTabsMenu.js, right? (I see a few addEventListeners utilizing the kElements that appear to correspond to the DOM elements in the .xhtml file, but I want to be sure)

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #2)

Hi Gijs!

I have two clarifying questions:

1. The 3 inline event handlers to remove, don't include line 10: gTabsPanel.searchTabs(), right?

Err, sorry, since I filed the bug someone has added a new item. So there are now 4. So yes, that one should be replaced, too.

2. I was initially concerned about whether I'd have to move the behavior elsewhere, but from what I can understand, that's already being done in browser-allTabsMenu.js, right? (I see a few addEventListeners utilizing the kElements that appear to correspond to the DOM elements in the .xhtml file, but I want to be sure)

Hm, so there are 4 toolbarbuttons in the markup that have oncommand listeners, and I don't see their id attributes in the JS file you mentioned.

I also don't see a command event listener generally.

command events do bubble up the DOM, so you can add a listener on the allTabsMenu-allTabsView that handles all 4 command events, using a switch on the event.target.id or similar. You can look at e.g. https://phabricator.services.mozilla.com/D207121 (which has already landed) for some inspiration, in case that helps?

Comment 4

[Leeya]

Okay, thank you for clarifying and for linking that code to help.

One more question, are there any tests I should run once I've made these changes?

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #4)

Okay, thank you for clarifying and for linking that code to help.

One more question, are there any tests I should run once I've made these changes?

Yes, running ./mach test browser/base/content/test/tabs/ should pass. :-)

Comment 6

[Leeya]

Hi Gijs!

Apologies for coming to you at the end of the day! I have two more questions:

1. Is there a particular default behavior that this switch should have?

2. I ran those tests before and after I updated my code and it looks good because I'm getting the same exact results, which is all tests passing but one. Again, that failed test was already failing to begin with, here's the Test Summary, is this okay?

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #6)

Hi Gijs!

Apologies for coming to you at the end of the day! I have two more questions:

1. Is there a particular default behavior that this switch should have?

Nope, you can leave out the default.

2. I ran those tests before and after I updated my code and it looks good because I'm getting the same exact results, which is all tests passing but one. Again, that failed test was already failing to begin with, here's the Test Summary, is this okay?

If it's the same before and after that sounds good. Please request review on your patch! :-)

Comment 8

[Leeya]

Hi Gijs,

No default for the switch statement, got it, and I'll be sure to add a reviewer.

I'm running into Mercurial issues and need help. Not sure how I got back on the central branch but I did and couldn't hg amend my new code. I ran hg wip to get my 6-digit-id and move to the right branch but it wasn't listed. I even scrolled through hg log and couldn't find it.

It had to have been created when I first ran hg commit -m "WIP..." and the first phabricator link was created, right? Do you know where else I can find it? I've looked over the Phab page carefully, but don't see it.

The last thing I tried to do was a commit to create a new branch, I figured I must not have had created the first one, but my terminal just hangs.
Here's that output, it was like that for 10 minutes before I CTRL+C'd.

Comment 9

[Leeya]

No default for the switch statement, got it, and I'll be sure to add a reviewer.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #8)

Hi Gijs,

No default for the switch statement, got it, and I'll be sure to add a reviewer.

I'm running into Mercurial issues and need help. Not sure how I got back on the central branch but I did and couldn't hg amend my new code. I ran hg wip to get my 6-digit-id and move to the right branch but it wasn't listed. I even scrolled through hg log and couldn't find it.

It had to have been created when I first ran hg commit -m "WIP..." and the first phabricator link was created, right? Do you know where else I can find it? I've looked over the Phab page carefully, but don't see it.

There's a "commits" tab and it has the hash of your commit: 878a4db91372. However, the phabricator patch that's up is empty (has no changes in it), so I'm not 100% sure what's happened there...

The last thing I tried to do was a commit to create a new branch, I figured I must not have had created the first one, but my terminal just hangs.
Here's that output, it was like that for 10 minutes before I CTRL+C'd.

I think it's hanging because it's got an ! in the line and that has a special meaning in the shell, if it's not inside single quotes. As you're using double quotes, the shell is interpreting it and it expects more input (that's what the > means).

You can leave off the ! or use string concatenation in the shell by using hg commit -m "something"'!' to put the ! in single quotes, which will escape it from the shell.

The other thing you can try is not give a commit message at all, and then mercurial will start your default editor (based on the HGEDITOR or EDITOR environment variable, or vim if neither is set) to let you set a commit message.

Comment 11

[Leeya]

Hi Gijs,

I don't see a "commits" tab on my Phab patch, but I think that's because it's empty...

I may have misunderstood Stephanie, she said that it was common for devs to immediately create a branch to work on by changing a small line of code and committing it as a WIP. And that uploading it via moz-phab would get the bug automatically assigned to us, which is what I was attempting to do...

I think I see where I went wrong, I should have left the change in code when I uploaded it to Phab so it wouldn't be empty.

Thank you for explaining how the ! is being interpreted, I really was at a loss!

Comment 12

[Leeya]

Attachment: Bug 1895490 - Remove inline event handlers from all tabs menu. r?Gijs!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ceaf3c87fb3d
Remove inline event handlers from all tabs menu. r=Gijs

Comment 14

[Leeya]

Hi Gijs! Once this patch lands, do you have another bug that you think I should tackle?

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #14)

Hi Gijs! Once this patch lands, do you have another bug that you think I should tackle?

I filed bug 1896764 for you. :-)

Comment 16

[Cristian Tuns]

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