Remove _getURITitleFromHistory from BookmarkProperties.js and disable the accept button until infos are collected.

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mano, Assigned: standard8)

Tracking

Trunk
Firefox 57
x86
macOS
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

The bookmark properties/addition dialog (the modal one) does some synchronous places data querying when it's opened (onload). It's mostly done in _determineItemInfo.

For this dialog to work with asynchronous APIs (the new getPlacesInfo api for example), we've two options:
(*) Open the dialog, then do the asynchronous queries. We'd have to disable everything in the dialog as we wait for the data to come in.
(*) Do it all in PlacesUIUtils before opening the dialog.

I think we should do the later, esp. given this dialog is modal.
Similar issues apply to the edit bookmark overlay.
Summary: bookmark properties/addition dialog: Query for places information before opening the dialog → edit-bookmark-overlay & bookmark properties/addition dialog: Query for places information before showing the UI
my only concern is tests using this dialog may start randomly failing if they are not properly handling this, though I agree the second option sounds better.
On a second thought, I'm going to go with the other option. From the user POV, the downside in the too-slow-case is pretty much the same: Either the dialog doesn't show up (the first option) or it shows up partially disabled (second option).

However the first option (load the data before opening the dialog) requires quite a lot more refactoring, and because we keep updating the fields through observers, the code would be duplicated in editBookmarkOverlay even in that case.

So, |disable; promise.then(enable);| is the way to go, IMO.
So this turned out to be the first issue raised by the fact that our transaction manager doesn't play nice with asynchronous code. I initially wrote a workaround patch for this bug, but it was too complex to our taste (me and Marco's, that is). So, until proven wrong, it seems we should rather go ahead and implement an asynchronous TM and fix this right. I'm doing that in bug 891303. If it turns out to be too large for Q3, I may revive the workaround patch.
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=5
Points: --- → 5
Whiteboard: p=5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 885241
OOps, _determineItemInfo still needs to be fixed.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
we still have the 2 options here, either disable the accept button until we are ready, or collect the information before opening the dialog. Probably we want the ui to react faster, so we may want to open the window earlier in a disabled state.
Blocks: 1385733
No longer depends on: PlacesAsyncTransact
Summary: edit-bookmark-overlay & bookmark properties/addition dialog: Query for places information before showing the UI → Remove _getURITitleFromHistory from BookmarkProperties.js and disable the accept button until infos are collected.
Assignee: nobody → standard8
Status: REOPENED → ASSIGNED
Comment on attachment 8895016 [details]
Bug 885246 - Delay enabling the accept button on the bookmarks properties dialog until after init is complete.

https://reviewboard.mozilla.org/r/166148/#review175564

I don't think the cancel button should be disabled.  You shouldn't have to wait for the async task to finish if you decide "never mind."  r+ with that change, or let me know why you disagree.

::: browser/components/places/content/bookmarkProperties.js:374
(Diff revision 1)
>            this._element("keywordField")
>                .addEventListener("input", this);
>          }
>        }
>      }
> +    acceptButton.disabled = acceptButtonDisabled;

Nit: This isn't an improvement over setting acceptButton.disabled directly IMO, but up to you.
Attachment #8895016 - Flags: review?(adw) → review+
Comment on attachment 8895020 [details]
Bug 885246 - Remove usage of the sync history.getPageTitle API from BookmarkProperties.js.

https://reviewboard.mozilla.org/r/166150/#review175566
Attachment #8895020 - Flags: review?(adw) → review+
Comment on attachment 8895016 [details]
Bug 885246 - Delay enabling the accept button on the bookmarks properties dialog until after init is complete.

https://reviewboard.mozilla.org/r/166148/#review175564

I was a bit concerned that we wouldn't stop the transaction properly part way through. However, it seems to be working reasonable well, so I'll leave the cancel available at the moment.

> Nit: This isn't an improvement over setting acceptButton.disabled directly IMO, but up to you.

I added a comment to explain it - I want to make sure we're only enabling the button once everything is done.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/baf4dd3acb2c
Delay enabling the accept button on the bookmarks properties dialog until after init is complete. r=adw
https://hg.mozilla.org/integration/autoland/rev/bb489c82da39
Remove usage of the sync history.getPageTitle API from BookmarkProperties.js. r=adw
https://hg.mozilla.org/mozilla-central/rev/baf4dd3acb2c
https://hg.mozilla.org/mozilla-central/rev/bb489c82da39
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.