Closed Bug 1122941 Opened 5 years ago Closed 5 years ago

taglist string lengths cut short

Categories

(Firefox :: Bookmarks & History, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- verified
firefox39 --- verified
firefox40 --- verified
firefox41 --- verified

People

(Reporter: wagle, Assigned: wagle, Mentored)

References

Details

(Keywords: regression, Whiteboard: [bugday-20150610])

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → Bookmarks & History
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.
Marco, any idea what caused this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mak77)
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(mak77)
Resolution: --- → WONTFIX
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.
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
Group: core-security
Restricted access to bug due to my being a little too forthcoming in my previous comment.  Sorry.
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).
Group: core-security
Good enough, couldn't do that myself, else would have done it that way.  Thanks.
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.
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>
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.
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!
(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
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.
Flags: needinfo?(mak77)
> 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.
I will remove the UI limit and uplift as far as possible.
Assignee: nobody → mak77
Status: REOPENED → ASSIGNED
Flags: needinfo?(mak77)
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.
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
Mentor: mak77
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.
Since we don't have data about what's the acceptable length, I'd just remove the limitation and let it go.
My first attempt at a firefox patch, how does it look?
Comment on attachment 8576571 [details] [diff] [review]
0001-Bug-1122941-Remove-bookmark-tags-field-max-length.-r.patch

set reviewer to mak77
Attachment #8576571 - Flags: review?(mak77)
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
Attachment #8576571 - Flags: review?(mak77) → review+
Attached patch redo of patchSplinter Review
did: git format-patch --no-stat -k master^..master

yeah, git-patch-to-hg-patch does copy the git-stats
Attachment #8576571 - Attachment is obsolete: true
Attachment #8576785 - Flags: checkin?(mak77)
Comment on attachment 8576785 [details] [diff] [review]
redo of patch

Thanks for the patch!
Attachment #8576785 - Flags: checkin?(mak77) → checkin+
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
Attachment #8576785 - Flags: approval-mozilla-beta?
Attachment #8576785 - Flags: approval-mozilla-aurora?
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.
Assignee: mak77 → wagle
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/7c899c6d817e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Attachment #8576785 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(mak77)
Attachment #8576785 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(mak77)
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.
Duplicate of this bug: 1154328
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
QA Whiteboard: [bugday-20150610]
Whiteboard: [bugday-20150610]
You need to log in before you can comment on or make changes to this bug.