1344769 - Cancel editing button loads BugZilla homepage instead of current bug

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: User Interface, defect)

Description

[James Ross]

User Agent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36 Edge/15.14986"

Steps to reproduce:
1. Open any bug page, e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1342944.
2. Log in if not already logged in.
3. Click "Edit Bug" in the top-right corner of the bug info.
4. Wait for bug to switch into editing mode.
5. Click "Cancel" button in same location.

Expected results:
Bug returns to non-editing mode.

Actual results: 
https://bugzilla.mozilla.org/ is loaded.

Cause:
The JavaScript on the cancel button does the following:
    event.preventDefault();
    window.location.replace($('#this-bug').val());

The matching element for $('#this-bug') is:
    <a id="this-bug" href="show_bug.cgi?id=1342944">Bug 1342944</a>

However, $('#this-bug').val() returns "" because there is nothing in an <a> element which makes sense for val(), so this is relying on undefined behaviour in jQuery AFAICT.

Comment 1

[:Gijs (he/him)]

Dylan, do we have a post-release tracker for the modal UI thing?

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

:Gijs, I'm using the whiteboard tag [modal-enhancement] to track post-release issues.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

That user agent string is confusing, but I believe the browser in question is Edge. I cannot reproduce in FFx Nightly (54.0a1) x64 on MacOS. 

I think an acceptable fix for this would be to reload the page instead of attempting to revert the view, especially if we're hitting browser dependent bugs.

Comment 4

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Augh, it's `[modal-enhancements]`, I should not answer needinfos before coffee. :)

Comment 5

[James Ross]

Yes, this is in Edge (Windows 10 Insider build 14986).

The code in question is actually trying to reload the page (and not just revert that view), but due to this difference in behaviour of jQuery's val() it ends up with a URL of "" instead of the expected "show_bug.cgi?id=1342944".

The simplest fix is likely to be to use jQuery's attr('href') to explicitly fetch the desired data instead of val(), which is evidently not reliable for this case.

Comment 6

[James Ross]

Oh, actually it seems that the jQuery val() is returning "" for all browsers (Firefox 51, Chrome 56) but the location.replace("") doesn't behave the same. Nevertheless, replacing the [now seemingly pointless?] val() with attr('href') or similar ought to fix it.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

I'm sure :seban can take care of this. I have Edge in a VM so it should be easy for me to review.

Comment 8

[:Atoll]

Technically, since no changes were submitted, shouldn't "Cancel" revert the changes to fields covered by the "Edit Bug" list and leave the rest of the fields intact?

Reloading the page risks comments being removed, and we can't really cheat the browser into doing the right thing with the Back button. I assume we could store original copies of the fields 'Edit Bug' reveals for edits if we don't already.

The assumption is that this "Cancel" button is only for the "Edit Bug" fields - right? Does it *also* intend to cancel all comments and needinfo and everything?  It's doesn't seem like it should be page-wide, but I always treated it as a risk to existing work.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: pull request

It works!

Comment 10

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   d1cf67fc2..48f4f832f  master -> master

Comment 11

[Mark Côté [:mcote]]

(In reply to Richard Soderberg [:atoll] from comment #8)
> Technically, since no changes were submitted, shouldn't "Cancel" revert the
> changes to fields covered by the "Edit Bug" list and leave the rest of the
> fields intact?
> 
> Reloading the page risks comments being removed, and we can't really cheat
> the browser into doing the right thing with the Back button. I assume we
> could store original copies of the fields 'Edit Bug' reveals for edits if we
> don't already.
> 
> The assumption is that this "Cancel" button is only for the "Edit Bug"
> fields - right? Does it *also* intend to cancel all comments and needinfo
> and everything?  It's doesn't seem like it should be page-wide, but I always
> treated it as a risk to existing work.

To answer this, yes, ideally that would be what it does.  We didn't opt for this initially because it's substantially more work and the current behaviour works as expected most of the time.  Not sure if there's a bug on file for "proper" cancellation.  There probably should be, but it'll be lowish priority, due to the work/benefit ratio.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

This is live now.