Closed Bug 1349823 Opened 7 years ago Closed 7 years ago

Bookmark panel doesn't immediately update fields when I open it, causing distracting text movement

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Version:
52 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 + wontfix
firefox54 + wontfix
firefox55 + verified

People

(Reporter: 684sigma, Assigned: bytesized)

References

Details

(Keywords: regression)

Attachments

(2 files)

Second scenario, by reporter.png
14.34 KB, image/png
Details
Bug 1349823 - Populate bookmark panel before showing it
59 bytes, text/x-review-board-request
enndeakin
: review+
mak
: review+
Details 
I have a problem with Firefox Beta 52, Beta 53. It happens on Nightly 55. Doesn't happen in Firefox ESR 45, Release 51.
Sometimes (always, it seems) when I open bookmark panel, its fields (namely "Name", "Folder", "Tags") update after the panel is shown, causing distracting text movement.
These 2 scenarios demonstrate the bug

First:
1. Open https://bugzilla.mozilla.org/show_bug.cgi?id=1345479 in a new ("first") tab, bookmark it with name "1".
2. Open https://bugzilla.mozilla.org/show_bug.cgi?id=1344309 in a new ("second") tab, bookmark it.
3. Assuming that the last time you opened bookmark panel in "second" tab, switch to "first" tab and press Ctrl+D.

Result: In the field "Name" the name of the 2nd bookmark is briefly displayed, but then it's replaced with "1"
Expected: All fields should initially display information related to current page


Second:
1. Open new window, open url https://bugzilla.mozilla.org/show_bug.cgi?id=1345479#1029392
2. Press Ctrl+D

Result: Initially "Page bookmarked" panel appears without any fields, then all fields appear
Expected: All fields should initially be displayed
Has STR: --- → yes
Keywords: regression
Short STR:
1) Open 2 tabs with random websites and bookmark both
2) Switch between both tabs and press Ctrl+D to display the bookmark panel

Result: Label 'name' for the current bookmark displays briefly the label 'name' of the previous bookmark.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0be513c03c8d04eb3a8fbe24bb11e3bbd01621f&tochange=8468a31dcb9441bbdd8a1f4f27a982409c677f0a

Regressed by bug 1206133.
Blocks: 1206133
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Ever confirmed: true
Flags: needinfo?(ksteuber)
This is definitely not a ship-stopping issue so not tracking for 52 and marking wontfix.
status-firefox52: affected → wontfix
tracking-firefox52: ? → ---
I know exactly where this problem comes from: http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/browser/base/content/browser-places.js#313

When panels were changed from opening synchronously to opening asynchronously, I needed to make many changes where calls that happened after panel-open calls now needed to use a callback so that they didn't execute before the panel opened.

That call to |gEditItemOverlay.initPanel| used to be invoked directly by |_doShowEditBookmarkPanel|, but now that panels open asynchronously, a callback is used so that |gEditItemOverlay.initPanel| is not called before the panel opens.

The underlying problem seems to be that if |gEditItemOverlay.initPanel| is called before the panel opens, the content is populated properly, but it seems that the event handlers are all not attached properly. The result of this is that the panel is populated before it opens, but you cannot close it with the Escape key.

I am not sure exactly where the line is that connects that event handler. I think I remember finding it when I was dealing with this last time, but I do not have the time to dig into it right now.

@enn Since you are more experienced than me in this area, can you offer any suggestions or insight?
Flags: needinfo?(ksteuber) → needinfo?(enndeakin)
It looks like some of the things that gEditItemOverlay.initPanel does should be done during popupshowing, specifically showing and hiding elements and initializing their values. Hooking up events and changing focus could be during popupshown.
Flags: needinfo?(enndeakin)
I can track this and please feel free to request uplift if you land a patch.  
Time is a little tight to get this into 53 so I'm marking this fix-optional for 53.
status-firefox53: affected → fix-optional
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox55: ? → +
I need clarifications about why ESC and similar events should not be working, those listeners are added by the panel() getter in browser-places.js, not by initPanel.
Off-hand the only broken thing looks like focusing, and we can probably split that by providing an event to initPanel to wait before focusing. Se could pass an optional "readyPromise" property to initPanel, and it could wait for that before doing any work that requires a specific ready state. Externally the promise could be resolved on popupShown while initPanel could be invoked at popupShowing.
Priority: -- → P2
Attachment #8855489 - Flags: review?(enndeakin)
Comment on attachment 8855489 [details]
Bug 1349823 - Populate bookmark panel before showing it

https://reviewboard.mozilla.org/r/127344/#review130226

::: browser/components/places/content/editBookmarkOverlay.js:351
(Diff revision 2)
>        // Clear the undo stack
> -      let editor = aElement.editor;
> -      if (editor)
> -        editor.transactionManager.clear();
> +      try {
> +        aElement.editor.transactionManager.clear();
> +      } catch (e) {
> +        // Ignore exceptions. |aElement.editor| may be null or the editor may
> +        // have no transaction manager. In either case, we don't need to worry

This needs a better explanation, in particular it should tell "when" that may happen.

Additionally, looks like if this happens we won't clear the transactionManager later, and that sounds like a bug. If we need to delay this to when the panel is ready, we should.

To do that, you could change onPanelReady to a promise (maybe make it default to a resolved promise) and use its thenable both in init and here.
Comment on attachment 8855489 [details]
Bug 1349823 - Populate bookmark panel before showing it

https://reviewboard.mozilla.org/r/127344/#review130918
Attachment #8855489 - Flags: review?(enndeakin) → review+
Comment on attachment 8855489 [details]
Bug 1349823 - Populate bookmark panel before showing it

Just to clarify comment 12
Attachment #8855489 - Flags: review-
Assignee: nobody → ksteuber
Hello and thanks for your input!

(In reply to Marco Bonardo [::mak] from comment #12)
> Additionally, looks like if this happens we won't clear the
> transactionManager later, and that sounds like a bug.

I'm not sure I really understand the transaction manager, so please bear with me. How does this cause the transaction manager to not be cleared later? Subsequent calls to this function should result in it being cleared just fine, right? The only time it won't get cleared is when it doesn't exist yet. Surely we don't need to wait for it to exist just to clear it; it should already be empty just after creation, right? What am I missing?
Flags: needinfo?(mak77)
(In reply to Kirk Steuber [:bytesized] from comment #15)
> I'm not sure I really understand the transaction manager, so please bear
> with me. How does this cause the transaction manager to not be cleared
> later? Subsequent calls to this function should result in it being cleared
> just fine, right? The only time it won't get cleared is when it doesn't
> exist yet. Surely we don't need to wait for it to exist just to clear it; it
> should already be empty just after creation, right? What am I missing?

My fear is that your catch is swallowing any error, not just the fact it may not have been created. It may cover future bugs.
I'd personally feel safer if we'd properly wait for clearing at the right time, or if there would be an if before the clear call for the specific case you are protecting from. The cost is low.
Can you predict that we won't ever implement an optimization where the transaction manager of an editor may be destroyed when the editor is not visible and recreated lazily when shown? I can't. And since the panel is reused and just reset, that would be a problem.
Flags: needinfo?(mak77)
I can prevent other errors from being swallowed by limiting the use of the try/catch to just the |editor.transactionManager| operation. I would prefer this fix to introducing a wait before clearing the transaction manager.

|_initTextField| is called in several other places by a number of other functions. The fix would involve changing many of them to accept a Promise as an additional, optional argument. I believe that this will make the system more complex and harder to reason about since it may not be obvious when the transaction manager will be cleared.

To me, it seems like it makes more sense to simplify things now than to plan for an optimization that we have no plans to implement. However, if you strongly believe that it makes more sense to introduce a wait period before clearing the transaction manager (or you suspect a future change to the transaction manager), I will yield to your opinion.

What do you think?
Flags: needinfo?(mak77)
(In reply to Kirk Steuber [:bytesized] from comment #17)
> I can prevent other errors from being swallowed by limiting the use of the
> try/catch to just the |editor.transactionManager| operation. I would prefer
> this fix to introducing a wait before clearing the transaction manager.

It wouldn't make much of a difference from now.
On the other side, would checking "if (editor && editor.transactionManager)" still solve your troubles, or is the clear() call that actually throws? I'd prefer the explicit if, if that works.
Flags: needinfo?(mak77)
The |clear()| call doesn't throw, accessing |editor.transactionManager| throws. I will change my patch to explicitly try only that operation.
Attachment #8855489 - Flags: review?(mak77)
My question is whether we throw because transactionManager is undefined/null, or if it actually throws because of its internal implementation.
If it throws, imo that's a bug in the editor, since transactionManager is defined as a read-only attribute, and I don't expect an attribute to throw, I'd expect it to be null, at a maximum. This is defined here, and nowhere it states that this may throw:
http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/editor/nsIEditor.idl#175

Indeed I suspect it's throwing here: http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/editor/libeditor/EditorBase.cpp#792 and (still imo) doesn't make sense for an attribute to throw instead of returning null.

I'll ni? Masayuki-san to see what he thinks about that, if we can fix the editor API I'd prefer to use a simple if check here, otherwise we'll pick the first version where we try/catch the whole thing (it should only catch for ex.result == Cr.NS_ERROR_FAILURE though), because this last version is even more unreadable :(
Flags: needinfo?(masayuki)
Hmm, AFAIK, I don't know the rule, attribute shouldn't return any exception.

In C++ level, some XPCOM methods return error when it cannot return valid pointer instead of returning nullptr with NS_OK. So, returning nullptr from nsIEditor::GetTransactionManager() might cause crash bugs. If you want to change it, you need to check call callers of m-c, c-c and BlueGriffon <https://github.com/therealglazou/bluegriffon/tree/bg2>.

I recommend that you should take it with a variable with try/catch if you want to uplift it.
Flags: needinfo?(masayuki)
Well, I don't think there's a specific strict rule, just that throwing from an attribute makes things ugly in js.
On the other side I understand your concern about existing cpp consumers. Fwiw, all of m-c consumers are doing it properly (null check the result), but let's not open that can of worms.

Kirk, please let's just revert to the first version of the patch with the try catch around the whole .clear() call, but please only catch NS_ERROR_FAILURE, in any other cases let's rethrow the exception.
We'll check users feedback for eventual issues.
I had to handle TypeError in addition to NS_ERROR_FAILURE to catch the error where the editor is null. I also kept the call to the |clear()| method out of the try/catch block since NS_ERROR_FAILURE is a pretty common error and it wasn't immediately clear when inspecting that function whether it might throw that error.

What do you think?
Flags: needinfo?(mak77)
Comment on attachment 8855489 [details]
Bug 1349823 - Populate bookmark panel before showing it

https://reviewboard.mozilla.org/r/127344/#review131510

OK, let's not further complicate our life :)
Attachment #8855489 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e4214720f24
Populate bookmark panel before showing it r=enndeakin+6102,mak
https://hg.mozilla.org/mozilla-central/rev/7e4214720f24
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
This feels a edge-case to me. What are your thoughts about possible backport, Kirk? Should we consider this for Aurora or can it ride the 55 train?
status-firefox53: fix-optional → wontfix
status-firefox-esr52: --- → wontfix
Flags: needinfo?(ksteuber)
I don't have particularly strong feelings about it. While I can see the bug that this fixes, I really have to be looking for it; it is not exactly obtrusive to me. However, it may be more noticeable for those with slower computers.

On one hand, it doesn't seem like a big deal to me if this rides the trains. On the other hand, it doesn't seem like a big risk to uplift either. I'm going to ask around a bit and see what a few other people think about uplifting this before I clear the needinfo.
I'm not hearing much interest in uplifting this. Let's just let it ride the trains.
Flags: needinfo?(ksteuber)
I have reproduced this bug with Nightly 55.0a1 (2017-03-22) on Windows 8.1 (64 bit).

This bug's fix is verified on Latest Nightly 55.0a1.

Build ID : 20170517030204
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday 20170517]
Status: RESOLVED → VERIFIED
While I do thank Azmina for helping us verify this on Windows 8.1 x64, we can't yet consider this VERIFIED until the fix is confirmed across platforms. Flagging this for further regression testing.
Status: VERIFIED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: qe-verify+
I reproduced this issue using Fx55.0a1, build ID: 20170322030208, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx55.0b3, on Windows 10 x64, macOS X 10.12.5 and Ubuntu 14.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: