1193621 - Can't change tag name in library / bookmarks manager

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Perry Wagle]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150807085045

Steps to reproduce:

Bookmarks -> Show All Bookmarks (to popup global bookmark editor)


Actual results:

clicked on tag, tried to change name, it ignored me


Expected results:

should have been able to change name (worked earlier today before I installed 40.0)

Comment 1

[:Gijs (he/him)]

Thank you for filing this!

Marco is away so I'll at least find a regression window if nobody else does.

Comment 2

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8a151d59bdec&tochange=d84b62b367b4

Regressed by:
d84b62b367b4	Asaf Romano — Bug 951651 - Make bookmarkProperties, Star UI and Library info pane work with PlacesTransactions. r=mak

Comment 3

[Marco Bonardo [:mak]]

IIRC this was done by Mano on purpose cause editing folder shortcuts titles was adding quite a bit of fragile code. But must be verified.

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #3)
> IIRC this was done by Mano on purpose cause editing folder shortcuts titles
> was adding quite a bit of fragile code. But must be verified.

Hm, but now there doesn't seem to be any way to edit tags at all anymore... except going to the individual bookmarks and updating all of them. That doesn't sound right... Mano, can you clarify what's going on here?

Comment 5

[Marco Bonardo [:mak]]

yes we need to fix this.

The wrokaround is to select all, add the new tag to all, then remove the old tag.

Comment 6

[Marco Bonardo [:mak]]

and likely this happens because we consider these read-only (they are QUERY_CONTAINING_QUERIES internally). I think we should be able to fix this, and write a test too.

Comment 7

[Perry Wagle]

> The wrokaround is to select all, add the new tag to all, then remove the old tag.

Wow..  And that now takes a lot of time, from 2 seconds for 100 bookmarks to 30 seconds or so.

Is that a separate bug?

Comment 8

[Marco Bonardo [:mak]]

I didn't say it's a solution, I said it's a workaround to obtain the same. This bug must be fixed.

Comment 9

[Perry Wagle]

Oh sorry, I meant that your workaround was faster in older versions of firefox (I used to do it normally when I needed to merge when tagnames collided).  This new slowdown might have something to do with the fix that created the main bug, hence my question in comment 7.

Comment 10

[Perry Wagle]

Actually, the slowdown seems to be general, I'm beachballing all over the place.  It's a separate bug.

Comment 11

[:Gijs (he/him)]

Marco, do you have time to look at this soon?

Comment 12

[Marco Bonardo [:mak]]

no, but I guess I should find it.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1

I still need to have a second look, but should do.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/editBookmarkOverlay.js
@@ +546,5 @@
> +        let guid = this._paneInfo.isTag
> +                    ? (yield PlacesUtils.promiseItemGuid(this._paneInfo.itemId))
> +                    : this._paneInfo.itemGuid;
> +        PlacesTransactions.EditTitle({ guid, title: newTitle })
> +                          .transact().catch(Components.utils.reportError);

driveby: if this is async, what happens if you type, and then you type again before the edit is complete? Or can't that happen here?

Comment 15

[Marco Bonardo [:mak]]

the function is called on blur

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

For now it should be fine, I should probably use Components.utils instead of Cu but I will fix that before pushing.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/editBookmarkOverlay.js
@@ +95,5 @@
>      // This pane is read-only if:
>      //  * the panel is not initialized
>      //  * the node is a folder shortcut
>      //  * the node is not bookmarked
>      //  * the node is child of a read-only container and is not a bookmarked URI

Should extend the comment here to mention tags.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6b84e7354d9b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b84e7354d9b

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Approval Request Comment
[Feature/regressing bug #]: bug 951651
[User impact if declined]: it's no more possible to edit a tag name in the Library
[Describe test coverage new/current, TreeHerder]: unit-test
[Risks and why]: nothing specific we can predict
[String/UUID change made/needed]: none

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 41+ as it's a bad regression.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Given that the risk assessment on this patch uplift request is not very reassuring, I would like to stabilize this on Aurora for a few days and then uplift to Beta41 if all goes well. Aurora42+ for now.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Perry, could you please verify this fix works as expected in 09/05 Nightly or later? Thanks in advance.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fa21e75f4ee

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out because the test was failing like https://treeherder.mozilla.org/logviewer.html#?job_id=1106730&repo=mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/2687ae28ea37

Comment 26

[Marco Bonardo [:mak]]

pff, the usual Array.prototype.includes problem in Aurora and Beta... it just needs to use .contains there (luckily this is the last merge with this issue). I will push the fixed patch tomorrow.

Comment 27

[Marco Bonardo [:mak]]

ehr, indexOf... this array/string thing is so boring.

Comment 28

[Perry Wagle]

Ritu : all my use cases work..

Thanks all!

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8654213 [details] [diff] [review]
patch v1

Given that the fix was verified, it seems safe to uplift to Beta41.

Comment 30

[Marco Bonardo [:mak]]

we need to reland to aurora a fixed patch first, not using .includes on arrays, the same should be landable on beta.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Marco, do you have an eta for this? The gtb of beta 8 is today and we would prefer take this patch in this beta instead of beta 9. Thanks

Comment 32

[Marco Bonardo [:mak]]

I was planning to do that now, but I'm not sure what today means, is there a time?

Comment 33

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9de325bc66b9

Comment 34

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-beta/rev/72e917cba6a7