The default bug view has changed. See this FAQ.

Comment box on the edit attachment page is too small

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Attachments & Requests
P2
normal
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: Conrad Carlen (not reading bugmail), Assigned: Guy Pyrzak)

Tracking

({ue})

2.15
Bugzilla 4.0
Bug Flags:
approval +

Details

(Whiteboard: [wanted-bmo])

Attachments

(7 attachments, 16 obsolete attachments)

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
Frédéric Buclin
: review+
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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

Comment 2

16 years ago
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.

Comment 3

16 years ago
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

Updated

15 years ago
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

Updated

12 years ago
Assignee: myk → attach-and-request
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Bugzilla 2.22 → ---

Updated

10 years ago
Priority: -- → P2

Updated

9 years ago
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]
Duplicate of this bug: 549066
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
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.

Comment 14

7 years ago
I would like to see a mockup with currently displayed fields.
(Assignee)

Comment 15

7 years ago
@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?
(Assignee)

Comment 16

7 years ago
Created attachment 429347 [details]
V2 of Editable Layout with fields from head
Attachment #429333 - Attachment is obsolete: true
Attachment #429339 - Attachment is obsolete: true
(Assignee)

Comment 17

7 years ago
Created attachment 429348 [details]
V2 of Editable Layout with fields from head Not Editable
(Assignee)

Updated

7 years ago
Attachment #429347 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #429348 - Attachment is obsolete: true
(Assignee)

Comment 18

7 years ago
Created attachment 429350 [details]
V2 of Editable Layout with fields from head Not Editable
(Assignee)

Comment 19

7 years ago
Created attachment 429351 [details]
V2 of Editable Layout with fields from head

Updated

7 years ago
Target Milestone: --- → Bugzilla 3.8
(Assignee)

Comment 20

7 years ago
Created attachment 429448 [details]
AttachmentView1
Attachment #429350 - Attachment is obsolete: true
Attachment #429351 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Created attachment 429449 [details]
patch view
(Assignee)

Comment 22

7 years ago
Created attachment 429450 [details]
is private attachment
(Assignee)

Comment 23

7 years ago
Created attachment 429451 [details]
Read Only View
(Assignee)

Comment 24

7 years ago
Created attachment 429452 [details] [diff] [review]
patch V1
Attachment #429452 - Flags: review?(mkanat)
(Assignee)

Comment 25

7 years ago
Created attachment 429453 [details]
when Edit is clicked in the Attachment details
(Assignee)

Updated

7 years ago
Attachment #429453 - Attachment description: Edit Attachment view → when Edit is clicked in the Attachment details

Comment 26

7 years ago
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-
(Assignee)

Comment 27

7 years ago
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.
(Assignee)

Comment 28

7 years ago
erm i meant js not css.
(Assignee)

Comment 29

7 years ago
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.
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)
(Assignee)

Comment 30

7 years ago
Created attachment 429484 [details]
comment on attachment with flags
(Assignee)

Comment 31

7 years ago
Created attachment 429485 [details]
Attachment as view only
(Assignee)

Comment 32

7 years ago
Created attachment 429486 [details]
JS turned off with view only
(Assignee)

Comment 33

7 years ago
Created attachment 429487 [details]
no JS edit attachment details
(Assignee)

Comment 34

7 years ago
Created attachment 429488 [details]
edit attachment details
(Assignee)

Comment 35

7 years ago
Created attachment 429489 [details]
view attachment
(Assignee)

Comment 36

7 years ago
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 37

7 years ago
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-
(Assignee)

Comment 38

7 years ago
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*

Comment 39

7 years ago
(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. :-)
(Assignee)

Comment 40

7 years ago
Created attachment 429662 [details] [diff] [review]
v3
Attachment #429481 - Attachment is obsolete: true
Attachment #429662 - Flags: review?(mkanat)

Comment 41

7 years ago
(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 42

7 years ago
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+
(Assignee)

Comment 43

7 years ago
Created attachment 429946 [details] [diff] [review]
Final Version

what i will actually commit
(Assignee)

Comment 44

7 years ago
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!
Attachment #429662 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #429662 - Attachment is obsolete: false
(Assignee)

Updated

7 years ago
Flags: approval?

Comment 45

7 years ago
The Submit button looks really wrong. Why is it square?

Comment 46

7 years ago
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?
(Assignee)

Comment 47

7 years ago
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.
Attachment #429946 - Attachment is obsolete: true
Attachment #429947 - Attachment is obsolete: true
Attachment #430039 - Flags: review?(LpSolit)
(Assignee)

Updated

7 years ago
Flags: approval?

Updated

7 years ago
Attachment #430039 - Attachment is patch: true
Attachment #430039 - Attachment mime type: application/octet-stream → text/plain

Comment 48

7 years ago
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-
(Assignee)

Comment 49

7 years ago
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 50

7 years ago
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?

Comment 51

7 years ago
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.

Comment 52

7 years ago
(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.

Comment 53

7 years ago
  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.)

Comment 54

7 years ago
Yes, I used "bzr merge /path/to/bundle".
(Assignee)

Comment 55

7 years ago
so i guess this patch stays in limbo? Can anyone on the cc list reproduce this?

Comment 56

7 years ago
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)

Comment 57

7 years ago
(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.

Updated

7 years ago
Flags: approval?
(Assignee)

Updated

7 years ago
No longer depends on: 365397
Duplicate of this bug: 365397

Comment 59

7 years ago
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+

Updated

7 years ago
Flags: approval+

Updated

7 years ago
Keywords: relnote
(Assignee)

Comment 60

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Attachment #430039 - Flags: review+

Updated

7 years ago
Attachment #429662 - Attachment is obsolete: true

Comment 61

7 years ago
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.