Closed Bug 885246 Opened 11 years ago Closed 7 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: asaf, Assigned: standard8)

References

Details

Attachments

(2 files)

Bug 885246 - Delay enabling the accept button on the bookmarks properties dialog until after init is complete.
59 bytes, text/x-review-board-request
adw
: review+
Details
Bug 885246 - Remove usage of the sync history.getPageTitle API from BookmarkProperties.js.
59 bytes, text/x-review-board-request
adw
: review+
Details 
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.
Blocks: 885247
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.
Depends on: 891303
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.
Blocks: fxdesktopbacklog
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=5
Points: --- → 5
Whiteboard: p=5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
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: 7 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: