1443189 - Misleading action involving the Delete option inside the Bookmarks panel

Status: RESOLVED FIXED

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

Description

[Anca Soncutean, Desktop QA]

[Affected versions]: 
- 60.0a1 (2018-03-05)

[Affected platforms]:
- Win 10 x64
- Ubuntu 16.0a x64
- macOS 10.13 

[Steps to reproduce]:
1. Launch Firefox
2. Bookmark several random pages
3. Go to the Library - Bookmarks
4. Right click on any site title inside the Recently Bookmarked area
5. Click on the Delete option

[Expected result]:
- The site is no longer bookmarked and it is instantly removed from the Recently Bookmarked list. 
- The user can delete any other site from the list.

[Actual result]:
- The site is no longer bookmarked, but it is not removed from the Recently Bookmarked list, until the Bookmark panel is refreshed. 
- The Delete option turns grey, no other removing action can be done ( https://goo.gl/BsEF9y ).

[Regression range]:
- Last good revision: 05a8ecb88424cbc51bc705f077ab41867f22619a
- First bad revision: 01e3d1408dbe8552f22ebb2f1d71d86e11bd7f14
- Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05a8ecb88424cbc51bc705f077ab41867f22619a&tochange=01e3d1408dbe8552f22ebb2f1d71d86e11bd7f14

[Additional notes]:
- The Recent History section is not affected by this behaviour.

Comment 1

[Anca Soncutean, Desktop QA]

Hi Paolo,

It seams like Bug 1439358 created the behavior mentioned above. Can you please take a look?

Comment 2

[:Paolo Amadini]

Thank you for the bug report and regression range! I have a patch ready to fix this.

Comment 3

[:Paolo Amadini]

Attachment: Bug 1443189 - Places subviews are not updated while they are open.

Review commit: https://reviewboard.mozilla.org/r/225284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225284/

Comment 4

[:Paolo Amadini]

When removing _isPopupOpen below, I missed that it was overriding a function from the base class.

https://hg.mozilla.org/integration/mozilla-inbound/rev/525b36047e16#l2.12

Comment 5

[:Gijs (he/him)]

Comment on attachment 8956399 [details]
Bug 1443189 - Places subviews are not updated while they are open.

https://reviewboard.mozilla.org/r/225284/#review231254

Please add a regression test.

Comment 6

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #4)
> When removing _isPopupOpen below, I missed that it was overriding a function
> from the base class.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/525b36047e16#l2.12

... whereas I looked at this when reviewing and assumed the base class was good enough after the changes to PMV...

Comment 7

[:Paolo Amadini]

Comment on attachment 8956399 [details]
Bug 1443189 - Places subviews are not updated while they are open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225284/diff/1-2/

Comment 8

[:Paolo Amadini]

I ended up placing the test in the "places" folder, since it was closer to the code that had to be fixed.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8956399 [details]
Bug 1443189 - Places subviews are not updated while they are open.

https://reviewboard.mozilla.org/r/225284/#review231728

Sweet, thanks!

Comment 10

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bae3738fb8883962421b046156c9a2a10339ba13

Comment 11

[:Paolo Amadini]

The test failed on Linux because the promise returned by PanelUI.show resolves during "popupshown" instead of ViewShown. New attempt:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5ca00006db7d0487c48b3049f1d2edda1798f95

There are a few existing tests that use the return value of PanelUI.show, and probably only work because of the specific timing of the test. I'll file a bug to remove the return value from PanelUI.show and add a test-only wrapper that waits for the correct event on the main view.

Comment 12

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #11)
> The test failed on Linux because the promise returned by PanelUI.show
> resolves during "popupshown" instead of ViewShown. New attempt:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f5ca00006db7d0487c48b3049f1d2edda1798f95
> 
> There are a few existing tests that use the return value of PanelUI.show,
> and probably only work because of the specific timing of the test. I'll file
> a bug to remove the return value from PanelUI.show and add a test-only
> wrapper that waits for the correct event on the main view.

There are already numerous wrappers. I'd prefer to get rid of them. Why not make PanelUI.show return the 'right' thing instead? :-\

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa987ae130e
Places subviews are not updated while they are open. r=Gijs

Comment 14

[:Paolo Amadini]

(In reply to :Gijs (under the weather; responses will be slow) from comment #12)
> Why not make PanelUI.show return the 'right' thing instead? :-\

Excluding tests, none of the consumers use the return value, not even UITour. Removing the return value would allow us to just remove the event listener from the function, since the implementation is not complete as the listener is never unregistered on failure.

We could move the event listening part to a CustomizableUITestUtils.for(window).showMainMenu() function that would be called by tests and screenshots code instead of PanelUI.show. This would be a simpler implementation compared to the same function in production because it can assume that opening the panel will not fail. Since it would be in a test-only module, test code across the tree could use it. We could add more functions to the test-only module on the same line, like CustomizableUITestUtils.for(window).hideMainMenu(). This would wait for the panel to close, whereas PanelUI.hide does not have a return value at present, and has to be wrapped manually in test code.

Alternatively, we could change the "show" function by listening for the "ViewShown" event and removing the listener if PanelMultiView.openPopup rejects or returns false, which I think is the "right" thing to return. This alone doesn't address the fact that we have to wrap the "hide" function in test code, unless we add a return value to it and mirror the logic of the "show" function in production code too, or keep the wrapping code in tests like it is now, but only for the hiding case.

I'd rather move this logic to test-only code, but we could also go the other way if you consider it better. Thoughts?

As an aside, can the anchor for the main menu panel be anything else than the main menu button?

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8aa987ae130e

Comment 16

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #14)
> (In reply to :Gijs (under the weather; responses will be slow) from comment
> #12)
> > Why not make PanelUI.show return the 'right' thing instead? :-\
> 
> Excluding tests, none of the consumers use the return value, not even
> UITour. Removing the return value would allow us to just remove the event
> listener from the function, since the implementation is not complete as the
> listener is never unregistered on failure.
> 
> We could move the event listening part to a
> CustomizableUITestUtils.for(window).showMainMenu() function that would be
> called by tests and screenshots code instead of PanelUI.show. This would be
> a simpler implementation compared to the same function in production because
> it can assume that opening the panel will not fail. Since it would be in a
> test-only module, test code across the tree could use it. We could add more
> functions to the test-only module on the same line, like
> CustomizableUITestUtils.for(window).hideMainMenu(). This would wait for the
> panel to close, whereas PanelUI.hide does not have a return value at
> present, and has to be wrapped manually in test code.
> 
> Alternatively, we could change the "show" function by listening for the
> "ViewShown" event and removing the listener if PanelMultiView.openPopup
> rejects or returns false, which I think is the "right" thing to return. This
> alone doesn't address the fact that we have to wrap the "hide" function in
> test code, unless we add a return value to it and mirror the logic of the
> "show" function in production code too, or keep the wrapping code in tests
> like it is now, but only for the hiding case.
> 
> I'd rather move this logic to test-only code, but we could also go the other
> way if you consider it better. Thoughts?

I don't really mind either way, I just know that we have oodles of utility functions for this stuff already:

https://dxr.mozilla.org/mozilla-central/search?q=promise%20panel%20show&=mozilla-central (and that's probably not all of them, some use promisePopupShown() but really are operating on a panel)

so if we write Yet Another Utility function then we best make sure we unify some of the test callsites, so we don't end up in the "there were 15 slightly different and separately complex ways of opening a panel in a test... and now there's 16" situation. It looks like some testcases use the identity popup as well though, so something that you can pass an optional panel (default to PanelUI.panel) might be helpful.

> 
> As an aside, can the anchor for the main menu panel be anything else than
> the main menu button?

I don't believe so, no.