Closed Bug 1344769 Opened 3 years ago Closed 3 years ago

Cancel editing button loads BugZilla homepage instead of current bug

Categories

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

Production
x86_64
Windows 10
defect
Not set
minor

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzilla-mozilla-20000923, Assigned: seban)

Details

(Whiteboard: [modal-enhancements])

Attachments

(1 file)

44 bytes, text/x-github-pull-request
dylan
: review+
Details | Review
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.
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)
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. :)
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.
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.
Attached file pull request
It works!
Attachment #8844671 - Flags: review+
To git@github.com:mozilla-bteam/bmo.git
   d1cf67fc2..48f4f832f  master -> master
Status: NEW → RESOLVED
Closed: 3 years 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
Component: User Interface: Modal → User Interface
You need to log in before you can comment on or make changes to this bug.