227.52 KB, image/png
213.55 KB, image/png
210.99 KB, image/png
233.51 KB, image/png
240.67 KB, image/png
257.78 KB, image/png
32.56 KB, patch
Frédéric Buclin: review+
Max Kanat-Alexander: review+
|Details | Diff | Splinter Review|
The recent feature of being able to edit attachments is great! Some problems though: (1) the comment field is too small. Couldn't the page layout be a bit different so it could be larger? Perhaps the same width as the attachment text but right below it? (2) when text is copied from the attachment comment field, it's not wrapped. It gets written onto the bug as one long line.
The second problem is a duplicate of bug 97784.
Any comment text fields need to be at least as wide as that on the edit bug page, which itself is too short for my liking.
Another throught. Rather than having two input fields on the edit attachment page (the Comment field, and the 'Edit attachment as comment' iframe), why not just have a single, big text field, and a button that can paste the attachment into that field if the user so desires. A button to view the attachment itself in a separate window would then be appropriate.
should probably deal with this, in whatever way it's going to be dealt with, in 2.16 since it's a "first impression" thing with the attachment manager. I find myself hitting the "edit as comment" button frequently, then just doing a select-all and delete and type my comment in the big box. I think I'd go for two boxes above one another that are the same width, one for comment and the other to view the attachment in.
I really did mean to retarget this...
We are currently trying to wrap up Bugzilla 2.16. We are now close enough to release time that anything that wasn't already ranked at P1 isn't going to make the cut. Thus this is being retargetted at 2.18. If you strongly disagree with this retargetting, please comment, however, be aware that we only have about 2 weeks left to review and test anything at this point, and we intend to devote this time to the remaining bugs that were designated as release blockers.
Enhancements which don't currently have patches on them which are targetted at 2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. Consideration will be taken for moving items back to 2.18 on a case-by-case basis (but is unlikely for enhancements)
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.
Not sure why I didn't find this bug when I was looking for it earlier... From bug 549066: I ran across http://coop.deadsquid.com/2010/02/growreviewcomment/ on Planet Mozilla today. I've been annoyed by this problem several times, and bug 528873 mentions people resorting to the use of userContent.css in order to fix this problem. While coop is using a greasemonkey script to get around this problem, I have been abusing the “Edit Attachment As Comment” button by erasing the contents and then writing what I need to. However, I shouldn’t have to do that either. Quoting from coop's post, "the textarea is 5 rows high and only 25 columns wide by default". That's ridiculous. We should fix this.
Created attachment 429333 [details] Mock of a new way to handle it? For your consideration a mock of how it could look. If i get a make it so (ie audience cheers maddly), I'll start working on it.
BTW, This same layout could be used for non,patches. we'd just apply the little (edit) hyperlink to the top area to make it expand.
Created attachment 429339 [details] New Layout for attachments Here it is! Comment and Attachment viewers grow with the page, sorry that's not clear in the mock. Not happy with where i put the patch and obsolete checkboxes but those can be moved easily, maybe up with the description stuff? None of this would effect the create attachment page.
I would like to see a mockup with currently displayed fields.
@Frederic I think the second mock shows that. All that changed was the addition of the edit link to the description stuff. Or do you mean you want to see it with the edit link clicked?
Created attachment 429347 [details] V2 of Editable Layout with fields from head
Created attachment 429448 [details] AttachmentView1
Created attachment 429452 [details] [diff] [review] patch V1
so like 70% of your comments can be answered with "was already there" there rest I have fixed. Also I changed up how i switch between the two UIs so it will no longer "require" css, just to be neato.
erm i meant js not css.
Created attachment 429481 [details] [diff] [review] patch v2 Addressed all but the "blockize" comment. That seemed like a bit much for this bug. Stay tuned for screenshots.
obsolete attachments are struck through (could add the words "obsolete" if needed. Private attachments are red and say "visible only to [% insider_group %]" Editing anything other than the comments or the flags requires clicking edit.
I'll change #bz_edit and #bz_hide to #bz_edit_action and #bz_hide_action. Basically some browsers (IE6!!!) are still pretty slow on getElementsByClassName. The rest should be an easy fix. I guess after this round of minor fixes we'll be ready to have a hugely improved attachments page *cheer*
(In reply to comment #38) > I'll change #bz_edit and #bz_hide to #bz_edit_action and #bz_hide_action. Those still sound like they could be pretty generic. > Basically some browsers (IE6!!!) are still pretty slow on > getElementsByClassName. Is that actually a problem on this page? > The rest should be an easy fix. I guess after this round of minor fixes we'll > be ready to have a hugely improved attachments page *cheer* Woot. :-)
Created attachment 429662 [details] [diff] [review] v3
(In reply to comment #39) > > Basically some browsers (IE6!!!) are still pretty slow on > > getElementsByClassName. > > Is that actually a problem on this page? IE6 being slow is not a showstopper. Being slow is not as bad as not working. I didn't look at the patches at all, but keep in mind that a logged-in user can still comment, even if he cannot edit attachment attributes.
Comment on attachment 429662 [details] [diff] [review] v3 This is so, so awesome. Fix the padding on the submit button, like we talked about on IM, and see if we can put the Submit button in the Flags div, so that it shows up roughly to the bottom-right of the comment box if the Flags listing is next to the comments. Also, re-attach as a bundle. But you can carry forward my r+.
Created attachment 429947 [details] [diff] [review] Final Version V2 Feel free to commit on my behalf. 1 adding the submit button where you suggested but I kinda think it looks funny. I've added the patch with both versions (v2 has it under the flags). Check it out, pick which one you prefer and commit it for me plz, thank you and... goodnight!
The Submit button looks really wrong. Why is it square?
Also, one of my installations has no attachment flags, and the Submit button is at the top right of the comment box. It should IMO be below the comment box, as for show_bug.cgi, for consistency. And this way, it won't be affected by the number of flags.
Created attachment 430039 [details] [diff] [review] Final Version V3 button wierdness fixed and put button where it should probably go, at the bottom of the form. We can file another bug if max REALLY wants it at the bottom right but it just didn't work out. I can explain why in more detail later.
Comment on attachment 430039 [details] [diff] [review] Final Version V3 When I first click "Edit as comment", then click "submit", the submit button moves slightly and changes are not committed. I have to reclick the button to make it work.
Ok, I officially NO clue what could cause this and I can't reproduce on the Apple, so I'm going to need someone on a linux box to debug it.
Comment on attachment 430039 [details] [diff] [review] Final Version V3 I actually can't reproduce LpSolit's problem on either Firefox or Chrome on Linux. LpSolit: Perhaps you didn't shift-refresh the page, so you had old JS or CSS?
One thing that might be nice, though, by the way, is to implement the "expand comment box when I click on it" preference for this page, now that the normal comments box is full-sized.
(In reply to comment #50) > Linux. LpSolit: Perhaps you didn't shift-refresh the page, so you had old JS or > CSS? I shift-reloaded the page. Note that the problem happens only if there is no attachment flag displayed at all. Once there is at least one flag in the UI, the problem goes away.
Hmm. I just tried on both Firefox and Chrome, and I still can't reproduce the problem. The patch is a bundle, did you apply it with merge? (It contains a rename.)
Yes, I used "bzr merge /path/to/bundle".
so i guess this patch stays in limbo? Can anyone on the cc list reproduce this?
Comment on attachment 430039 [details] [diff] [review] Final Version V3 LpSolit: Neither pyrzak nor I can reproduce the issue you're seeing--could we check this in and file a separate bug for further investigation?
(In reply to comment #56) > LpSolit: Neither pyrzak nor I can reproduce the issue you're seeing--could we > check this in and file a separate bug for further investigation? Hum no, I would prefer to fix this now. Note that you must be logged in as a powerless user to reproduce the problem. If you have privs to edit attachment attributes (either because you have editbugs privs, or because you are the attachment creator) then the problem doesn't occur.
Comment on attachment 430039 [details] [diff] [review] Final Version V3 OK, I spent some more time testing this patch with some other browsers, and I can only reproduce the issue with my current profile on Linux. If I use another profile, it works; if I use Google Chrome, Opera or Konqueror, it works. So a Firefox extension must be interacting with the UI, maybe. Anyway, the patch now passes all my testing, so r=LpSolit
galley:~/Sites/dharma/bugzilla pyrzak$ bzr commit --fixes mozilla:101770 -m"Bug 101770 - Comment box on the edit attachment page is too small \n r=mkanat/lpSolit, a=lpsolit " Committing to: bzr+ssh://email@example.com/bugzilla/trunk/ modified js/attachment.js renamed skins/standard/create_attachment.css => skins/standard/attachment.css modified template/en/default/attachment/create.html.tmpl modified template/en/default/attachment/diff-header.html.tmpl modified template/en/default/attachment/edit.html.tmpl modified template/en/default/attachment/show-multiple.html.tmpl modified template/en/default/bug/create/create.html.tmpl Committed revision 7049.
Added to the release notes in bug 604256.