1198978 - Pressing "Save" when no jobs are selected on the pinboard clears the classification/comment

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit a TreeHerder page, e.g. https://treeherder.mozilla.org/#/jobs?repo=mozilla-central

 2. Click some job.
 3. Click "Open Pinboard".

 4. (note that your job is not actually selected in the pinboard yet)
 5. Type in a comment and click "Save"

EXPECTED RESULTS: Nothing should happen (or perhaps an error message should appear), because "Save" is disabled, because no jobs are selected.

ACTUAL RESULTS: The comment I typed in gets cleared!!

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug

Sorry, my STR were missing a step:
 (0) Be logged into TreeHerder (so that you can post comments).

(If you aren't logged in and perform my STR, you *do* get a helpful message telling you that you need to log in before you can submit comments -- and the comment field isn't cleared, which is nice. But after you log in and repeat the STR, it does get cleared.)

Here's a screencast demonstrating the bug. I'm using current Firefox nightly on 64-bit linux, if it matters.

Comment 2

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

I can take this and hopefully have time to work on it Friday morning. If I don't get to it then, I won't get to it until at least Tuesday. Feel free to unassign me if someone can get to this earlier than that. :)

Comment 3

[Ed Morley [:emorley]]

Thank you for filing - agree this is suboptimal UX.

Comment 4

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

Attachment: PR 925

I think this should do it. 

Unsure if I need to also add this check to saveClassificationOnly() and saveBugsOnly().

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8654151 [details] [review]
PR 925

Redirecting to Cameron, since UK public holiday today.

Comment 6

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

Comment on attachment 8654151 [details] [review]
PR 925

And back to Ed. :)

Comment 7

[Ed Morley [:emorley]]

Comment on attachment 8654151 [details] [review]
PR 925

Taking this since Cameron is on PTO and Wes asked about it on IRC.

I think the better approach here would be to disable the save button instead (it makes more sense to see it disabled, rather than press it and not have anything happen and wonder why).

...but Cameron may have a better suggestion for this (I'm not that familiar with the pinboard).

Comment 8

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

Hrm, the Save button is ng-disabled when there's no pinned jobs [1], but the button itself is a <span> element, not an <input> [2], so I guess disabling it doesn't prevent the click event from being fired?


1. https://github.com/mozilla/treeherder/blob/62f23396680f14cf075dedea2cb427531eb95038/ui/partials/main/thPinboardPanel.html#L73
2. https://github.com/mozilla/treeherder/blob/62f23396680f14cf075dedea2cb427531eb95038/ui/partials/main/thPinboardPanel.html#L70

Comment 9

[Ed Morley [:emorley]]

Yeah I see what you mean. There seem to be some suggestions here:

https://www.google.co.uk/search?q=angular+ng-disabled+click+event

Also the styling of the save button (specifically the text colour / button colour) could be made a little more obviously disabled.

Comment 10

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

Comment on attachment 8654151 [details] [review]
PR 925

Okay, that seems to work locally, and it didn't break Travis, so here we go.

Comment 11

[Cameron Dawson [:camd]]

Comment on attachment 8654151 [details] [review]
PR 925

I hate to give you another r-.  Nothing personal.  I like you as a human being, still!  :)

Also, that was a clever trick that I'll place in the back of my head to use sometime.  :)

Comment 12

[Daniel Holbert [:dholbert]]

This bit me again today; I'd love to see this fixed!

Comment 13

[William Lachance (:wlach)]

Do you have time to finish this Wes? I can take a look if you're too swamped.

Comment 14

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

Please do!

Comment 15

[William Lachance (:wlach)]

Ok, I'll take this. :)

Comment 16

[crosscent]

@Wlach. I am new to this whole thing, but is it possible for me to take over this? I think I have a solution for this.

Comment 17

[William Lachance (:wlach)]

(In reply to crosscent from comment #16)
> @Wlach. I am new to this whole thing, but is it possible for me to take over
> this? I think I have a solution for this.

Yes, if you want to do this feel free -- I haven't started yet. You can use :KWierso's pull request (attached) as a basis for your work, just bear in mind the suggested changes that :camd made.

Comment 18

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #17)
> (In reply to crosscent from comment #16)
> > @Wlach. I am new to this whole thing, but is it possible for me to take over
> > this? I think I have a solution for this.
> 
> Yes, if you want to do this feel free -- I haven't started yet. You can use
> :KWierso's pull request (attached) as a basis for your work, just bear in
> mind the suggested changes that :camd made.

Also, you've probably already seen this, but you can find out more than probably ever wanted to know about treeherder here:

https://wiki.mozilla.org/EngineeringProductivity/Projects/Treeherder

In particular if you have questions feel free to pop onto IRC (#treeherder channel on irc.mozilla.org)

Comment 19

[crosscent]

(In reply to William Lachance (:wlach) from comment #18)
> (In reply to William Lachance (:wlach) from comment #17)
> > (In reply to crosscent from comment #16)
> > > @Wlach. I am new to this whole thing, but is it possible for me to take over
> > > this? I think I have a solution for this.
> > 
> > Yes, if you want to do this feel free -- I haven't started yet. You can use
> > :KWierso's pull request (attached) as a basis for your work, just bear in
> > mind the suggested changes that :camd made.
> 
> Also, you've probably already seen this, but you can find out more than
> probably ever wanted to know about treeherder here:
> 
> https://wiki.mozilla.org/EngineeringProductivity/Projects/Treeherder
> 
> In particular if you have questions feel free to pop onto IRC (#treeherder
> channel on irc.mozilla.org)

Thanks wlach! I am not too sure what the workflow should be, but I have sent a PR already. If I missed a step, please let me know!

Comment 20

[William Lachance (:wlach)]

Attachment: PR

Comment 21

[William Lachance (:wlach)]

(In reply to crosscent from comment #19)

> Thanks wlach! I am not too sure what the workflow should be, but I have sent
> a PR already. If I missed a step, please let me know!

Creating a new PR is the right idea, but it needs to be formatted a certain way and attached to the bug (normally autolander would do this automatically, but in your case it didn't because it wasn't formatted properly, so I did this for you). When you've addressed my comments in the PR, please set the review flag (also described in the PR) and we can get this resolved!

Comment 22

[crosscent]

(In reply to William Lachance (:wlach) from comment #21)
> Creating a new PR is the right idea, but it needs to be formatted a certain
> way and attached to the bug (normally autolander would do this
> automatically, but in your case it didn't because it wasn't formatted
> properly, so I did this for you). When you've addressed my comments in the
> PR, please set the review flag (also described in the PR) and we can get
> this resolved!

I have created a new commit! I am assuming that the review flag is the eyes at the bottom, and I have clicked it to be green. I assume that this will be reviewed by someone else later, but I am not too sure, so I have not pressed the publish button yet.

Comment 23

[William Lachance (:wlach)]

(In reply to crosscent from comment #22)
> (In reply to William Lachance (:wlach) from comment #21)

> I have created a new commit! I am assuming that the review flag is the eyes
> at the bottom, and I have clicked it to be green. I assume that this will be
> reviewed by someone else later, but I am not too sure, so I have not pressed
> the publish button yet.

You need to set the review flag here in bugzilla. :) I've already attached the PR to this bug for you, so all you need to do is click on "details" on the PR attachment above, then set the review combo box to r? and select me (:wlach) as the reviewer.

There are a few changes I requested in the PR. Also, you should squash all the intermediary commits into one via a git rebase. Once that's done, select review according to the instructions above. I know this seems like a lot of process at first but I promise it will eventually seem like second nature.

Comment 24

[crosscent]

(In reply to William Lachance (:wlach) from comment #23)

> You need to set the review flag here in bugzilla. :) I've already attached
> the PR to this bug for you, so all you need to do is click on "details" on
> the PR attachment above, then set the review combo box to r? and select me
> (:wlach) as the reviewer.
> 
> There are a few changes I requested in the PR. Also, you should squash all
> the intermediary commits into one via a git rebase. Once that's done, select
> review according to the instructions above. I know this seems like a lot of
> process at first but I promise it will eventually seem like second nature.

I have rebased and squash all intermediary commits into one. I had to force push it due to rebasing, and it seems that I have to create a new PR? I'm sorry for taking such a long time to to make a proper PR for such a small change. I really hope I am doing this correct, I am actually really happy that you are willing to help me through this process!

Comment 25

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

As long as you pushed to the same branch, the pull request should just accept it. It'll invalidate any comments left to the original commits, but that should be fine.

Comment 26

[William Lachance (:wlach)]

Comment on attachment 8734386 [details] [review]
PR

Looks great now, thanks!

Comment 27

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla/treeherder/commit/45376b5d24af5629f373bd8739c0f052981a6a08

Comment 28

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/590d688eadebaa0a6c5726fcbffdc6634c9b140d
Bug 1198978 - Disable save button if no job

* Changed span to button
* added tooltip for disabled button

Comment 29

[William Lachance (:wlach)]

This broke keyboard input after classification on Firefox on Linux/Windows, for some reason which I don't understand. I think this is a platform bug, but I don't have time to do a minimal test case right now, so let's just hack around it.

Comment 30

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1198978-followup > mozilla:master

Comment 31

[William Lachance (:wlach)]

Comment on attachment 8735997 [details] [review]
[treeherder] wlach:1198978-followup > mozilla:master

Hey Cam, I wish I had a better solution for this but I don't know of one. :(

Comment 32

[Cameron Dawson [:camd]]

Comment on attachment 8735997 [details] [review]
[treeherder] wlach:1198978-followup > mozilla:master

wfm.  :)

Comment 33

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/e04077e64ffd1041f653696874d8e77b08e9f8fe
Bug 1198978 - Fix keyboard input after job classification

For some reason changing the span to a button broke keyboard input
after job classification on Firefox on Linux/Windows (not Mac,
any flavor of Chrome also works fine). Blurring the input element
seems to fix this.

https://github.com/mozilla/treeherder/commit/566c0318adf41ee65a4bac4e0e8b1af8b329a48e
Merge pull request #1376 from wlach/1198978-followup

Bug 1198978 - Fix keyboard input after job classification

Comment 34

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

Camd pushed the fix to stage and it appears to work correctly for me on Firefox on Windows and Linux. Holding off on pushing to prod at just before 5pm because that's almost never the right thing to do. 

If anyone's stuck trying to sheriff tonight on Firefox on Windows or Linux and is getting caught by this, running the following snippet in a web console tied to your current treeherder tab will temporarily fix it until you reload the page:
document.getElementById("pinboard-controls").getElementsByTagName("button")[0].addEventListener("click", function() {document.activeElement.blur()}, false)


You'll have to do it separately for each treeherder tab, but it unbreaks my muscle memory, which is always a good thing. :)

Comment 35

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

This got pushed to production this morning and seems to be working.