1122941 - taglist string lengths cut short

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Perry Wagle]

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

Steps to reproduce:

With the "Show All Bookmarks" editor, tried to edit an URL with the tags:

020 --------------------------------------------------------, 024 --------------------------------------------------------, 028 --------------------------------------------------------, 032 --------------------------------------------------------, 052 --------------------------------------------------------, 060 --------------------------------------------------------, 072 --------------------------------------------------------, 080 --------------------------------------------------------, 094 --------------------------------------------------------, 096 --------------------------------------------------------, 099 --------------------------------------------------------, 100 --------------------------------------------------------, 101 --------------------------------------------------------, 102 --------------------------------------------------------, 104 --------------------------------------------------------, 110 --------------------------------------------------------, 120 --------------------------------------------------------, 130 --------------------------------------------------------, 200 --------------------------------------------------------, 211 --------------------------------------------------------, 222 --------------------------------------------------------, 300 --------------------------------------------------------, 333 --------------------------------------------------------, 390 --------------------------------------------------------, 399 --------------------------------------------------------, 400 --------------------------------------------------------, 404 --------------------------------------------------------, 408 --------------------------------------------------------, 412 --------------------------------------------------------, 416 --------------------------------------------------------, 480 --------------------------------------------------------, 484 --------------------------------------------------------, 488 --------------------------------------------------------, 492 --------------------------------------------------------, 496 --------------------------------------------------------, 520 --------------------------------------------------------, 522 --------------------------------------------------------, 526 --------------------------------------------------------, 530 --------------------------------------------------------, 540 --------------------------------------------------------, 542 --------------------------------------------------------, 548 --------------------------------------------------------, 550 --------------------------------------------------------, 552 --------------------------------------------------------, 554 --------------------------------------------------------, 556 --------------------------------------------------------, 558 --------------------------------------------------------, 560 --------------------------------------------------------, 562 --------------------------------------------------------, 564 --------------------------------------------------------, 566 --------------------------------------------------------, 568 --------------------------------------------------------, 570 --------------------------------------------------------, 572 --------------------------------------------------------, 596 --------------------------------------------------------, 620 --------------------------------------------------------, 640 --------------------------------------------------------, 650 --------------------------------------------------------, 900 --------------------------------------------------------, 904 --------------------------------------------------------, 990 --------------------------------------------------------, 999 --------------------------------------------------------, aaa --------------------------------------------------------



Actual results:

New chars are ignored.  Pastes are ignored.  Replacing highlighted chars is ignored.

Deleting chars is not ignored.


Expected results:

The tag list should be editable no matter what the string length

If you paste the above taglist, it is truncated a few chars past the tag starting with "102".

This worked fine a few weeks ago, so was probably introduced with 35.

Its a real work stopper for me, since I visually depend on the tags to serve as separators between groups of tags.

Comment 1

[Perry Wagle]

This makes it impossible to edit the tags.  The database stores taglists that long just fine.  Not sure why someone would decide to limit the number of chars.

Comment 2

[Dietrich Ayala (:dietrich)]

Marco, any idea what caused this?

Comment 3

[Marco Bonardo [:mak]]

we are limiting tags to 100 chars, if a bigger tag is passed to the API, it throws.
This changed in Firefox 35.
Just reduce length of your "separators" so they are smaller than 100 chars.

Comment 4

[Perry Wagle]

The length of the LIST of tags is limited to about half the size of my current lists in the database.  I don't think any of the individual tags are 100 chars.

Comment 5

[Perry Wagle]

Since I didn't notice people getting email for my objection to the accessment of just what the bug is, I'm reopening.  Sorry if this is rude.

Comment 6

[Marco Bonardo [:mak]]

yes, the text field is limited to 1000 chars and your content above is about 4000... I can see us increasing that a little bit, but with good reasons.

Could you please better explain to me your use-case, I'm not sure I understand your separators usage off-hand, the example in comment 0 looks very exotic.

Comment 8

[Perry Wagle]

Restricted access to bug due to my being a little too forthcoming in my previous comment.  Sorry.

Comment 9

[Jeff Walden [:Waldo]]

Reopening bug, marking comment 7 private for the reasons noted in comment 8.  Hopefully that satisfies the need without making the bug totally hidden (particularly, from the developers who might see the bug and be willing to invest time in fixing it).

Comment 10

[Perry Wagle]

Good enough, couldn't do that myself, else would have done it that way.  Thanks.

Comment 11

[Perry Wagle]

TL;DR version of comment 7:

I have over 1000 tags, and I need to divide them into groups, mostly sorted by a 3 digit numerical prefix.  Years of experience with this have shown me that the visual separators between groups are mandatory.  After a few months of one system, I grow inured, and need to change things around.  To do this, I need to be able to move groups around, and with this change, I can't renumber the separators.

Comment 12

[Jeff Walden [:Waldo]]

There's not UI, but I imagine mass changes that would take weeks to perform using the UI, could be accomplished significantly more quickly with a custom extension that made bookmark modifications directly acting upon the underlying database.  Granted, that'd require extra work on your part to find someone to write it, and to pay for it.  But it might be worth the time/cost to avoid weeks of work, if weeks of work is ever needed.

As a moderately disinterested observer, it's not clear to me why tags should have limited length.  But I won't going to go out of my way to argue it.  And the relatively unusual (I hope we would generally agree) outlier use case presented in comment 7 is a bit too niche a reason to change that, as far as I'm concerned.

</resume-silence>

Comment 13

[Perry Wagle]

Each separator is a tag about 60 chars long.  The LIST of tags is over 1000 chars long.  This used to work fine.  Now it was changed.  The handicapped on-ramp is closed.

Comment 14

[Perry Wagle]

What was fixed that caused tags to be forcefully truncated?  Where do I find the patches?  You used to be able to edit long lists of tags just fine; if you leave the UI broken like this, then at some point someone is going to make the DB follow suit.  And the DB currently stores these tag lists just fine.

Thanks!

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to Perry Wagle from comment #13)
> Each separator is a tag about 60 chars long.  The LIST of tags is over 1000
> chars long.  This used to work fine.  Now it was changed.  The handicapped
> on-ramp is closed.

Bug 407821 restricted individual *tag* length to 100 chars. The reporter's tags are 60 chars. No problem there.

This bug is that the *tag list* in the Bookmarks Organizer is being truncated.

That is a very different thing - the impact being that users are now restricted to the number of tags per URL... which is a very different constraint than tag length and doesn't seem to be tied to the problem laid out in bug 407821.

Marco, why are we restricting the length of the sum total of tags per bookmark to 1000 chars?

Perry, for now you could crack open your Firefox and revert the first change in the patch to bug 407821:

https://bug407821.bugzilla.mozilla.org/attachment.cgi?id=8490732

Comment 16

[Dietrich Ayala (:dietrich)]

Some more analysis after reading the comments in that bug:

1. The performance complaints are all about long *individual* tags. There's nothing in that bug that talks about a UI slowdown for many tags under 100 chars but which sum to greater than 1000 chars total. We should have someone test that it does or does not have the same effect that they're seeing with giant single tags.

2. I don't really see anything in that bug which narrows down the amount of text at which the UI actually gets too slow. We have guidelines for what "too slow" is, such as research done by our UX team (eg page 10 of https://mozilla.app.box.com/s/aww17rx74k7fjds5vada ). But in that bug, they just picked an arbitrary number :P

3. The patch also didn't make the tagging APIs in Places respect this new sum-of-1000-chars-for-all-tags-per-bookmark constraint. It can easily be broken by adding two tags via the API which sum to greater than 1000 chars.

Comment 17

[Perry Wagle]

> Perry, for now you could crack open your Firefox and revert the first change in the patch to bug 407821

Its not in my firefox app, will try to recompile firefox by this weekend.

Comment 18

[Marco Bonardo [:mak]]

I will remove the UI limit and uplift as far as possible.

Comment 19

[Perry Wagle]

Just so you know, I wasn't sleepy, so I built gecko from git in 34 minutes.  Decided to do vanilla for the first pass.  Turns out to be the nightly version.  Which version did you want me to modify?  What testing?

Unless of course, Marco DOES end up doing this.  In which case, I thank Marco in advance.

Comment 20

[Marco Bonardo [:mak]]

I'm very happy if you wish to do the change by yourself, I'll gladly mentor you along the process!
This can help
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

The version you need to modify is trunk (mozilla-central), or you can start from the fx-team integration branch.
Manual testing is fine, we didn't have a test for the original change that limited the field length.
If you have a patch up before the end of the week, that'd be great.
For any question feel free to ask here (needinfo me) or ask in the #introduction channel on irc.mozilla.org

Comment 21

[Perry Wagle]

Ok, well, I patched it to max at 10000 chars as that would be less likely to cascade trouble, and that worked, albeit being very slow with Nightly.  Don't see you in #introduction, though...

Right now, I'll switch to trunk.

Comment 22

[Marco Bonardo [:mak]]

Since we don't have data about what's the acceptable length, I'd just remove the limitation and let it go.

Comment 23

[Perry Wagle]

Attachment: 0001-Bug-1122941-Remove-bookmark-tags-field-max-length.-r.patch

My first attempt at a firefox patch, how does it look?

Comment 24

[Perry Wagle]

Comment on attachment 8576571 [details] [diff] [review]
0001-Bug-1122941-Remove-bookmark-tags-field-max-length.-r.patch

set reviewer to mak77

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8576571 [details] [diff] [review]
0001-Bug-1122941-Remove-bookmark-tags-field-max-length.-r.patch

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

I think you must still edit config of the git tools so they don't add diff stats into the commit message, here I see

---
 browser/components/places/content/editBookmarkOverlay.xul | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

I think there is a --no-stat option in git-format-patch and it's likely (didn't verify) mirrored in the moz-git-tools

Apart this the patch looks good, please just fix the commit message before setting the checkin-needed keyword

Comment 26

[Perry Wagle]

Attachment: redo of patch

did: git format-patch --no-stat -k master^..master

yeah, git-patch-to-hg-patch does copy the git-stats

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8576785 [details] [diff] [review]
redo of patch

Thanks for the patch!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7c899c6d817e

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 8576785 [details] [diff] [review]
redo of patch

Approval Request Comment
[Feature/regressing bug #]: bug 407821
[User impact if declined]: Users with many tags could encounter unexpected errors and dataloss if they hit the limit we imposed, by losing tags.
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: very simple patch that only removes a maxlength from a xul element, basically reverting the code at what it was before. no risk.
[String/UUID change made/needed]: none

Comment 30

[Marco Bonardo [:mak]]

Thanks for fixing the bug Perry, the positive thing about this story is that now you know how to contribute to Firefox code, so you can continue helping us improving the product and making it better for you and the others.
If you wish to look for other simple bugs you can take a look at
https://developer.mozilla.org/en-US/docs/Introduction#Step_3_-_Find_something_to_work_on
Or you can freely roam in bugzilla, there's plenty of action around.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c899c6d817e

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8576785 [details] [diff] [review]
redo of patch

I think it's fine to revert this behaviour that has caused user problems. This also reverts the fix for the issue reported in bug 407821.

Mak - Instead of removing the limit, should we look for a way to keep it but increase it? Are there other ways to handle bug 407821?

Comment 33

[Marco Bonardo [:mak]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #32)
> Mak - Instead of removing the limit, should we look for a way to keep it but
> increase it? Are there other ways to handle bug 407821?

It's not easy to find what might be a good limit that won't hurt users, and the API doesn't have a limit on the number of tags a bookmark can have.
At a maximum we could add telemetry to verify the current numbers in the wild.

Comment 34

[Marco Bonardo [:mak]]

and, I'm not sure it's worth spending time on tags for now, until we decide how we plan to support their usage in future bookmarking UI.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87fc4129c84e

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3836553057c4

Comment 38

[Ashickur Rahman]

Successfully reproduce the bug on Firefox 38.0a1 , build id: 20150124030202

Fix works for me on : 

Firefox 38.0.5 Mac, Build ID: 20150525141253 , User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0

Firefox 39.0 Windows, Build ID: 20150609130336 , User Agent: Mozilla/5.0 (Windows NT 6.1; rv:39.0) Gecko/20100101 Firefox/39.0

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

Firefox 41.0a1 Windows, Build ID: 20150612030205, User Agent: Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0