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

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: crosscent)

Tracking

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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!!
(Reporter)

Comment 1

3 years ago
Created attachment 8653115 [details]
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.
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. :)
Assignee: nobody → wkocher

Comment 3

3 years ago
Thank you for filing - agree this is suboptimal UX.
Priority: -- → P3
Summary: My carefully-entered classification / comment gets clobbered if I accidentally click "Save" when no builds are selected → Pressing "Save" when no jobs are selected on the pinboard clears the classification/comment
Created attachment 8654151 [details] [review]
PR 925

I think this should do it. 

Unsure if I need to also add this check to saveClassificationOnly() and saveBugsOnly().
Attachment #8654151 - Flags: review?(emorley)

Comment 5

3 years ago
Comment on attachment 8654151 [details] [review]
PR 925

Redirecting to Cameron, since UK public holiday today.
Attachment #8654151 - Flags: review?(emorley) → review?(cdawson)
Comment on attachment 8654151 [details] [review]
PR 925

And back to Ed. :)
Attachment #8654151 - Flags: review?(cdawson) → review?(emorley)

Comment 7

3 years ago
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).
Attachment #8654151 - Flags: review?(emorley) → review-
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
Flags: needinfo?(emorley)

Comment 9

3 years ago
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.
Flags: needinfo?(emorley)
Comment on attachment 8654151 [details] [review]
PR 925

Okay, that seems to work locally, and it didn't break Travis, so here we go.
Attachment #8654151 - Flags: review- → review?(cdawson)
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.  :)
Attachment #8654151 - Flags: review?(cdawson) → review-
(Reporter)

Comment 12

3 years ago
This bit me again today; I'd love to see this fixed!
Do you have time to finish this Wes? I can take a look if you're too swamped.
Flags: needinfo?(wkocher)
Please do!
Assignee: wkocher → nobody
Flags: needinfo?(wkocher)
Ok, I'll take this. :)
Assignee: nobody → wlachance
(Assignee)

Comment 16

3 years ago
@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.
(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.
Assignee: wlachance → crosscent
(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)
(Assignee)

Comment 19

3 years ago
(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!
(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!
(Assignee)

Comment 22

3 years ago
(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.
(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.
(Assignee)

Comment 24

3 years ago
(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!
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.
(Assignee)

Updated

3 years ago
Attachment #8734386 - Flags: review?(wlachance)
Keywords: autoland
Comment on attachment 8734386 [details] [review]
PR

Looks great now, thanks!
Attachment #8734386 - Flags: review?(wlachance) → review+

Updated

3 years ago
Keywords: autoland

Comment 28

3 years ago
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

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8735997 [details] [review]
[treeherder] wlach:1198978-followup > mozilla:master
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. :(
Attachment #8735997 - Flags: review?(cdawson)
Comment on attachment 8735997 [details] [review]
[treeherder] wlach:1198978-followup > mozilla:master

wfm.  :)
Attachment #8735997 - Flags: review?(cdawson) → review+

Comment 33

3 years ago
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
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. :)
This got pushed to production this morning and seems to be working.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.