Cancel editing button loads BugZilla homepage instead of current bug

VERIFIED FIXED

Status

()

bugzilla.mozilla.org
User Interface: Modal
--
minor
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: James Ross, Assigned: seban)

Tracking

Production
x86_64
Windows 10

Details

(Whiteboard: [modal-enhancements])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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

a year ago
Dylan, do we have a post-release tracker for the modal UI thing?
Component: Bug Creation/Editing → User Interface: Modal
Flags: needinfo?(dylan)
:Gijs, I'm using the whiteboard tag [modal-enhancement] to track post-release issues.
Flags: needinfo?(dylan)

Updated

a year ago
Whiteboard: [modal-enhancement]
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.
Whiteboard: [modal-enhancement] → [modal-enhancements]
Augh, it's `[modal-enhancements]`, I should not answer needinfos before coffee. :)
(Reporter)

Comment 5

a year ago
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.
(Reporter)

Comment 6

a year ago
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.
I'm sure :seban can take care of this. I have Edge in a VM so it should be easy for me to review.
Assignee: nobody → sebastinssanty
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.
Created attachment 8844671 [details] [review]
pull request

It works!
Attachment #8844671 - Flags: review+
To git@github.com:mozilla-bteam/bmo.git
   d1cf67fc2..48f4f832f  master -> master
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(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.
This is live now.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.