Closed Bug 101770 Opened 23 years ago Closed 14 years ago

Comment box on the edit attachment page is too small

Categories

(Bugzilla :: Attachments & Requests, defect, P2)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: ccarlen, Assigned: guy.pyrzak)

References

Details

(Keywords: ue, Whiteboard: [wanted-bmo])

Attachments

(7 files, 16 obsolete files)

227.52 KB, image/png
Details
213.55 KB, image/png
Details
210.99 KB, image/png
Details
233.51 KB, image/png
Details
240.67 KB, image/png
Details
257.78 KB, image/png
Details
32.56 KB, patch
LpSolit
: review+
mkanat
: 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.
Summary: attachment comment formatting problems → comment box on the edit attachment page should be bigger
Depends on: 102957
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...
Target Milestone: --- → Bugzilla 2.16
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Component: Creating/Changing Bugs → attachment and request management
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)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Assignee: myk → attach-and-request
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Bugzilla 2.22 → ---
Priority: -- → P2
Depends on: 365397
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.
Severity: enhancement → normal
Keywords: ue
Summary: comment box on the edit attachment page should be bigger → Comment box on the edit attachment page is too small
Whiteboard: [wanted-bmo]
Attached image Mock of a new way to handle it? (obsolete) —
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.
Attached image New Layout for attachments (obsolete) —
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?
Attachment #429333 - Attachment is obsolete: true
Attachment #429339 - Attachment is obsolete: true
Attachment #429347 - Attachment is obsolete: true
Attachment #429348 - Attachment is obsolete: true
Target Milestone: --- → Bugzilla 3.8
Attached image AttachmentView1 (obsolete) —
Attachment #429350 - Attachment is obsolete: true
Attachment #429351 - Attachment is obsolete: true
Attached image patch view (obsolete) —
Attached image is private attachment (obsolete) —
Attached image Read Only View (obsolete) —
Attached patch patch V1 (obsolete) — Splinter Review
Attachment #429452 - Flags: review?(mkanat)
Attachment #429453 - Attachment description: Edit Attachment view → when Edit is clicked in the Attachment details
Comment on attachment 429452 [details] [diff] [review]
patch V1

>=== modified file 'js/attachment.js'
>+function toggle_attachment_details_visibility ( ) 
>+{
>+    // show hide classes
>+    if( YAHOO.util.Dom.hasClass(edit, 'bz_default_hidden') ){
>+        YAHOO.util.Dom.removeClass(edit, 'bz_default_hidden');
>+        YAHOO.util.Dom.addClass(readonly, 'bz_default_hidden');        
>+    } else {
>+        YAHOO.util.Dom.removeClass(readonly, 'bz_default_hidden');
>+        YAHOO.util.Dom.addClass(edit, 'bz_default_hidden');              
>+    }

  Not that this is really important, but isn't that kind of just bz_toggleClass, twice?

>=== modified file 'skins/standard/create_attachment.css'

  This should be renamed to just plain-old "attachment.css".

>+#smallCommentFrame, #attachment_flags {
>+	  float: left;

  There's a tab there, and in a few other places in the file.

>+#attachment_information_read_only .details {
>+	  font-family: "Courier New", Courier, monospace;
>+}

  Mmm, any reason to explicitly pick a monospace font instead of just using the browser's default? I think on my machine (and Linux in general) the default may actually be actually nicer than Courier New.

>+.bz_private {
>+    border: 1px solid #F8C8BA;
>+}

  Hmm, is this something that can go into global.css?

>+.checkboxes div{
>+    float: left;
>+}

  That seems like a pretty generic name, maybe it should be limited to a container id?

>=== modified file 'template/en/default/attachment/edit.html.tmpl'
>+  <div class="attachment_info" width="100%">

  I know that the "width" was there in the old version, but can we get rid of it anyhow?

>+      <div id="attachment_attributes">
>+        <div id="attachment_information_read_only" class="[% " bz_private" IF attachment.isprivate%]">

  Nit: The space in front of bz_private actually isn't necessary here. Also, class="" won't be necessary either, so maybe we should wrap the whole class attribute in the IF.

>+          <div class="title">
>+            [% "[patch]" IF attachment.ispatch%]
>+            [% attachment.description %]

  I think you need a "FILTER html" there.

>+        <div id="attachment_information_edit" class="bz_default_hidden">

  That makes the page require JavaScript. I'm OK with that, I suppose.

>+          <div id="attachment_description">
>+            <label for="description">Description:</label>
>+            [% INCLUDE global/textarea.html.tmpl
>+              id             = 'description'
>+              name           = 'description'
>+              minrows        = 3
>+              cols           = 25

  Should we make the cols wider? I know that it's controlled by CSS and all now, though...so does it even matter?

>+            <div id="attachment_ispatch">
>+              <input type="checkbox" id="ispatch" name="ispatch" value="1"
>+                     [%+ IF !can_edit %]class="bz_hidden_option"[% END %]

  bz_hidden_option is really only for the value-controller code. I'd use bz_default_hidden here.

  Also, it seems like if can_edit is false, this input just shouldn't be being displayed at all, no? (Same comment for the other checkboxes, when it comes to can_edit.)

>+                     [%+ 'checked="checked"' IF attachment.ispatch %]>
>+                <label for="ispatch">patch</label>

  Hmm, so the checkbox is hidden, but the text isn't, if we're !can_edit?

>+                [% IF can_edit %]
>+                  <label for="isprivate">private (only visible to
>+                    <strong>[% Param('insidergroup') FILTER html %]</strong>)

  I mentioned this to you on IRC, but the name of the insidergroup should probably be highlighted somehow, since this text is already bolded by the CSS.

>+                  </label>
>+                [% ELSE %]
>+                  <span class="label">Is Private:</span>

  This should have the explanatory text on it, as well, I think. (Although it's only in very rare situations that you'd see this--you'd have to be in a product that had canedit set, which is almost never used.)

>+        <div id="attachment_view_window">

  While we're here, could you put this in a BLOCK? Maybe even section-ize the page, like we did for show_bug.

>+                    The attachment is not viewable in your browser due to security
>+                    restrictions enabled by [% terms.Bugzilla %].

  While we're here (you don't have to do this in this bug, but I noticed it now and it's a small thing), we should say "by your Bugzilla administrator" instead of "by Bugzilla"--otherwise people think that this is some unchangeable thing.

>+                  [% INCLUDE global/textarea.html.tmpl
>+                    id      = 'editFrame'
>+                    name    = 'comment'
>+                    style   = 'height: 400px; width: 100%; display: none'

  Oh man, style is the devil. :-) (I know that it was here before.) Maybe this should go into the CSS file now that we have one?

>+                  <iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;">

  style eeees the deeevillllll! :-) (I know it was already here. :-) )

>+                  <script type="text/javascript">
>+                    <!--
>+                    var patchviewerinstalled = 0;
>+                    var attachment_id = [% attachment.id %];
>+                    if (typeof document.getElementById == "function") {
>+        [% IF use_patchviewer %]

  What's up with that weird spacing? I'm guessing that was there before? (It's there in a few other places too.)

>+                      var patchviewerinstalled = 1;
>+                      document.write('<iframe id="viewDiffFrame" style="height: 400px; width: 100%; display: none;"><\/iframe>');

  There's the style attribute again. :-)

>+        <div id="attachment_comments_and_flags">
>+        [%IF user.id %]

  Nit: Space after the first %.

>+        </div>
>+        <div style="clear:both; padding-bottom: 1.5em">

  *a soft hissing can be heard in the distance, perhaps it is the leaky air conditioner of hell, or simply the touch of the devil's feet on lovely new HTML* :-)



  So, did I mention that you rock? This is awesome. Not only is it spiffy newness and a great new UI, but it's all divs!! You rock.
Attachment #429452 - Flags: review?(mkanat) → review-
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.
Attached patch patch v2 (obsolete) — Splinter Review
Addressed all but the "blockize" comment. That seemed like a bit much for this bug. Stay tuned for screenshots.
Assignee: attach-and-request → guy.pyrzak
Attachment #429448 - Attachment is obsolete: true
Attachment #429449 - Attachment is obsolete: true
Attachment #429450 - Attachment is obsolete: true
Attachment #429451 - Attachment is obsolete: true
Attachment #429452 - Attachment is obsolete: true
Attachment #429453 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #429481 - Flags: review?(mkanat)
Attached image Attachment as view only
Attached image edit attachment details
Attached image view attachment
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.
Comment on attachment 429481 [details] [diff] [review]
patch v2

  So, I was thinking--for logged-out users or users who can't edit, why not just display the UI without the (edit) link in it, and not show the form at all? That would keep the logged-out UI consistent with the logged-in UI, and I like the format of the header that you created that shows up before you click on "(edit)".

  Also, for the comment box, the box itself is supposed to turn red, not the area around it. (That's what show_bug.cgi does, is turn the comment box red.)

>=== renamed file 'skins/standard/create_attachment.css' => 'skins/standard/attachment.css'
>+#smallCommentFrame{

  Nit: Space before {.

>+    margin-right: 10px;

  Why is the margin in px instead of em?

>+#attachment_information_read_only .title #bz_edit {

  It seems like bz_edit should be a class, not an id, right? That sounds like something that could easily be accidentally duplicated.

>+.no_javascript #bz_hide, .no_javascript #bz_edit {

  I think it would just be better to put bz_default_hidden on those items, and then remove it directly.

>=== modified file 'template/en/default/attachment/edit.html.tmpl'
> [snip]
>+            <input type="text" size="20"  class="text block[% editable_or_hide %]"

  I guess attribute selectors don't work in IE6, right? (Otherwise this would be one of the world's most obvious uses for them.)

>+      <div id="attachment_comments_and_flags">
>+      [% IF user.id %]
>+        <div id="smallCommentFrame" class="[% "bz_private" IF attachment.isprivate%]">

  Nit: Space before the % at the end, there.
Attachment #429481 - Flags: review?(mkanat) → review-
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. :-)
Attached patch v3 (obsolete) — Splinter Review
Attachment #429481 - Attachment is obsolete: true
Attachment #429662 - Flags: review?(mkanat)
(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+.
Attachment #429662 - Flags: review?(mkanat) → review+
Attached patch Final Version (obsolete) — Splinter Review
what i will actually commit
Attached patch Final Version V2 (obsolete) — Splinter Review
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!
Attachment #429662 - Attachment is obsolete: true
Attachment #429662 - Attachment is obsolete: false
Flags: approval?
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.
Flags: approval?
Attached patch Final Version V3Splinter Review
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.
Attachment #429946 - Attachment is obsolete: true
Attachment #429947 - Attachment is obsolete: true
Attachment #430039 - Flags: review?(LpSolit)
Flags: approval?
Attachment #430039 - Attachment is patch: true
Attachment #430039 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #430039 - Flags: review?(LpSolit) → review-
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?
Attachment #430039 - Flags: review- → review?(LpSolit)
(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.
Flags: approval?
No longer depends on: 365397
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
Attachment #430039 - Flags: review?(LpSolit) → review+
Flags: approval+
Keywords: relnote
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://guy.pyrzak%40gmail.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #430039 - Flags: review+
Attachment #429662 - Attachment is obsolete: true
Added to the release notes in bug 604256.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: