Hide (instead of showing as disabled) flags that aren't available for a specific component on enter_bug.cgi

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Attachments & Requests
P2
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: shaver, Assigned: dkl)

Tracking

({ue})

3.6.1
Bugzilla 4.4
Bug Flags:
approval +

Details

Attachments

(2 attachments, 6 obsolete attachments)

Created attachment 459901 [details]
screenshot

According to beltzner, the config says that they should only appear for Core:nanojit and Tamarin:*.  See attached screenshot, they're shown on new bug entry but aren't manipulable.

Updated

7 years ago
Assignee: create-and-change → nobody
Component: Creating/Changing Bugs → Bugzilla: Other b.m.o Issues
Product: Bugzilla → mozilla.org
QA Contact: default-qa → other-bmo-issues
Version: unspecified → other
LpSolit: Why is this a bmo bug? This is a very old Bugzilla issue.
Assignee: nobody → attach-and-request
Component: Bugzilla: Other b.m.o Issues → Attachments & Requests
Keywords: ue
OS: Windows 7 → All
Product: mozilla.org → Bugzilla
QA Contact: other-bmo-issues → default-qa
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.6
Version: other → 3.6.1
Summary: flashplayer flags showing up when filing in Core:Widget Win32 → Hide (instead of showing as disabled) flags that aren't available for a specific component on enter_bug.cgi
Target Milestone: Bugzilla 3.6 → Bugzilla 4.0

Comment 2

7 years ago
(In reply to comment #1)
> LpSolit: Why is this a bmo bug?

Because the bug summary was clearly about bmo. :) And you can fix it locally on bmo if that's a problem for you.

From what I remember when I implemented this feature in bug 174039, myk and I chose to disable flags rather than hide them to avoid the text to jump when new flags (dis)appear. pyrzak, what's your opinion on this?
Severity: normal → enhancement
Target Milestone: Bugzilla 4.0 → ---
Created attachment 459987 [details] [diff] [review]
patch - v1
Assignee: attach-and-request → reed
Status: NEW → ASSIGNED
Attachment #459987 - Flags: review?(mkanat)
Attachment #459987 - Flags: review?(guy.pyrzak)
(In reply to comment #2)
> (In reply to comment #1)
> > LpSolit: Why is this a bmo bug?
> 
> Because the bug summary was clearly about bmo. :) And you can fix it locally on
> bmo if that's a problem for you.

Sure, but you could have asked shaver why he thought this was something in Bugzilla rather than bmo and/or even look at the screenshot. Just saying.

Also, I shouldn't have to write custom code for bmo just to give users some semblance of a reasonable user experience.

> From what I remember when I implemented this feature in bug 174039, myk and I
> chose to disable flags rather than hide them to avoid the text to jump when new
> flags (dis)appear.

It's too confusing how it is now. I remember when that bug was implemented, and I disagreed with the change then, but it hasn't really impacted me much until we started having to have all these various Flash-based and Weave-based flags that are only in a few components out of an entire product. It becomes very annoying and takes up a lot of room.
Target Milestone: --- → Bugzilla 4.0
Comment on attachment 459987 [details] [diff] [review]
patch - v1

Doesn't work just right. New patch coming up.
Attachment #459987 - Attachment is obsolete: true
Attachment #459987 - Flags: review?(mkanat)
Attachment #459987 - Flags: review?(guy.pyrzak)

Comment 6

7 years ago
Hmm. I understand the concern about things moving around when you change the component. I think, though, that since usually people aren't very near the flags interface when they're choosing a component, and they almost always will be choosing the component before setting the flags, it would be OK to have them appear and disappear.

Of course, if pyrzak disagrees or has some better idea, I'd love to hear it. :-)

Comment 7

7 years ago
I often have debates with co-workers about disable vs disappear. Here is where the debate usually arrives to:

1. Disabling lets users know that the option exists and for some reason it is currently not enabled (think Paste).
2. Disappear makes sure users are not confused by random things that there is no way they could interact with and just take up visual space.

These types of flags are hard because it is quite easy to change components and then have flags appear disappear which could be confusing. However, if BMO is not as inclined to change components then it might be different.

I know there once was a bug to redo the way the flags UI should work so that users would just have a drop down of possible flags to pick from and then select ?, +, - from that and then after the save that flag would become "real". This was a more complicated bug, but this solution seems more appropriate either way since it:

1. Cleans up the UI
2. Allows users to see disabled flags without the clutter of the current UI.

So I guess my answer is, you should fix that other bug (bug 424215).

Now if you guys think bug 424215 is too hard or not worth waiting for/working on. Then I guess my pref is just to add and remove a css class which then BMO can set to display:none.

Comment 8

7 years ago
(In reply to comment #7)
> These types of flags are hard because it is quite easy to change components and
> then have flags appear disappear which could be confusing.

  Well, except that you start off with no component selected, so you start out with disabled flags, which seems confusing to me.

> I know there once was a bug to redo the way the flags UI should work so that
> users would just have a drop down of possible flags to pick from and then
> select ?, +, - from that and then after the save that flag would become "real".

  Yeah. Although would that really be better for most installations, which have only one or two flags?

> Then I guess my pref is just to add and remove a css class which then BMO
> can set to display:none.

  Why just bmo? Why not add bz_default_hidden to them?

Comment 9

7 years ago
bz_default_hidden would cause them to be hidden all the time, which i thought was the "hard decision", to hide them or set them to disabled. Or was that an incorrect assessment?

Comment 10

7 years ago
(In reply to comment #9)
> bz_default_hidden would cause them to be hidden all the time, which i thought
> was the "hard decision", to hide them or set them to disabled. Or was that an
> incorrect assessment?

  Well, I meant that we'd toggle bz_default_hidden on and off of them. 

  Well, I don't think the decision is too difficult, from my perspective. I mean, I don't see strong enough reasons to show them but have them disabled. Do you?

Comment 11

7 years ago
Nah, feel free to hide them.

Updated

7 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2

Updated

7 years ago
Target Milestone: Bugzilla 4.2 → Bugzilla 5.0
(Assignee)

Comment 12

6 years ago
Created attachment 583305 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v1)

With the commit made recently to hide the flag rows on show_bug.cgi, it is now a little easier to accomplish this with enter_bug.cgi as well. Here is a patch 
that hides the full flag row that is not needed for the component selected.

Please review.
dkl
Assignee: reed → dkl
Attachment #583305 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #583305 - Flags: review? → review?(LpSolit)

Comment 13

6 years ago
Comment on attachment 583305 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v1)

>+        var table = YAHOO.util.Dom.get("flags");

There is no element with id="flags". There is id="bug_flags" and id="attachment_flags". id="flags" is only used in show_bug.cgi for existing bugs.


>+        YAHOO.util.Event.addListener('bz_flags_more_action', 'click', function (e) {
>+            YAHOO.util.Dom.addClass('bz_flags_more_container', 'bz_default_hidden');
>+            for (var i = 0; i < rows.length; i++) {
>+                YAHOO.util.Dom.removeClass(rows[i], 'bz_default_hidden');
>             }
>-        }

There is no "more flags" link in enter_bug.cgi, and so no element with id="bz_flags_more_container".


>+        // We show the flag rows available for the selected component.
>+        var table = YAHOO.util.Dom.get("flags");
>+        var rows = YAHOO.util.Dom.getElementsByClassName('bz_flag_type', 'tbody', table);

You already defined both variables above. And as above, there is no element with id="flags".


>+            var flagSelect = YAHOO.util.Dom.getElementsBy(function(el) {
>+                return el.name.search(/^flag_type-/) != -1;
>+            }, 'select', rows[i])[0];

I don't understand what you are trying to do here. We already have a list of flagtypes elements, in rows.


>+                if (flagSelect.name.search("^flag_type-" + flags[index][j] + "$") != -1) {
>+                    flagSelect.disabled = false;
>+                    YAHOO.util.Dom.removeClass(rows[i], 'bz_default_hidden');
>+                    break;
>+                } else {
>+                    flagSelect.disabled = true;
>+                    YAHOO.util.Dom.addClass(rows[i], 'bz_default_hidden');
>+                }

Why flipping the "disabled" attribute? We want to show/hide flags now, not enable/disable them.
Attachment #583305 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 14

6 years ago
(In reply to Frédéric Buclin from comment #13)
> Comment on attachment 583305 [details] [diff] [review]
> Patch to hide flag rows for when not applicable to component selected (v1)
> 
> >+        var table = YAHOO.util.Dom.get("flags");
> 
> There is no element with id="flags". There is id="bug_flags" and
> id="attachment_flags". id="flags" is only used in show_bug.cgi for existing
> bugs.
> 
> 
> >+        YAHOO.util.Event.addListener('bz_flags_more_action', 'click', function (e) {
> >+            YAHOO.util.Dom.addClass('bz_flags_more_container', 'bz_default_hidden');
> >+            for (var i = 0; i < rows.length; i++) {
> >+                YAHOO.util.Dom.removeClass(rows[i], 'bz_default_hidden');
> >             }
> >-        }
> 
> There is no "more flags" link in enter_bug.cgi, and so no element with
> id="bz_flags_more_container".
>  
> >+        // We show the flag rows available for the selected component.
> >+        var table = YAHOO.util.Dom.get("flags");
> >+        var rows = YAHOO.util.Dom.getElementsByClassName('bz_flag_type', 'tbody', table);
> 
> You already defined both variables above. And as above, there is no element
> with id="flags".

Ugh. Sorry. All above was some copy over from another JS file I was using as reference and forgot to remove it before submitting.
 
> >+            var flagSelect = YAHOO.util.Dom.getElementsBy(function(el) {
> >+                return el.name.search(/^flag_type-/) != -1;
> >+            }, 'select', rows[i])[0];
> 
> I don't understand what you are trying to do here. We already have a list of
> flagtypes elements, in rows.

rows is a list of tbody elements and not the actual select elements that I need to work with. getElementsBy searches for 'select' elements that are children of the row 'tbody' element and returns the first one if the name matches the criteria.

> 
> >+                if (flagSelect.name.search("^flag_type-" + flags[index][j] + "$") != -1) {
> >+                    flagSelect.disabled = false;
> >+                    YAHOO.util.Dom.removeClass(rows[i], 'bz_default_hidden');
> >+                    break;
> >+                } else {
> >+                    flagSelect.disabled = true;
> >+                    YAHOO.util.Dom.addClass(rows[i], 'bz_default_hidden');
> >+                }
> 
> Why flipping the "disabled" attribute? We want to show/hide flags now, not
> enable/disable them.

You have to flip the disabled attribute so that if they set a flag that is only valid for componentX but then select componentY later before submitting the bug without un-setting the previously valid flag. If you don't disable it, you will get an error message at the top of the show_bug.cgi page stating that some flags were not able to be set. If you disable, then no message occurs.

dkl
(Assignee)

Comment 15

6 years ago
Created attachment 590354 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v2)

Thanks for the feedback. New patch with fixes.

dkl
Attachment #583305 - Attachment is obsolete: true
Attachment #590354 - Flags: review?(LpSolit)

Comment 16

6 years ago
Comment on attachment 590354 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v2)

>+        // We show or hide the available flags depending on the selected component.
>+        var table = YAHOO.util.Dom.get("bug_flags");

You also have to show/hide attachment flags. Else you get:
  "Some flags could not be set. Please check your changes."


>+        for (var i = 0; i < rows.length; i++) {

>+            for (var j = 0; j < flags[index].length; j++) {

I'm not too happy with a loop within a loop. This means that time spent here will vary proportionnally to N^2 instead of N currently. But maybe JS is fast enough so that the perf decrease is negligible?


>+                } else {
>+                    flagSelect.disabled = true;
>+                    YAHOO.util.Dom.addClass(rows[i], 'bz_default_hidden');
>+                }

Instead of executing these two lines again and again on a flag which is probably already disabled, you should rather set some variable to true/false, and only execute these two lines when you are done with the loop, depending on the value of this variable. This would be much faster.


>-            // Do not enable flags the user cannot set nor request.
>-            if (flagField && flagField.options.length > 1) {

This feature has been removed with your patch. If you cannot +/-/? a flag, do not display it at all.
Attachment #590354 - Flags: review?(LpSolit) → review-

Updated

6 years ago
Priority: -- → P2

Comment 17

6 years ago
dkl: ping?
(Assignee)

Comment 18

5 years ago
Created attachment 631535 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v3)

New simplified patch that addresses your suggested changes.

dkl
Attachment #590354 - Attachment is obsolete: true
Attachment #631535 - Flags: review?(LpSolit)

Comment 19

5 years ago
Comment on attachment 631535 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v3)

dkl said a new patch was needed.
Attachment #631535 - Flags: review?(LpSolit)
(Assignee)

Comment 20

5 years ago
Created attachment 636744 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v4)

New patch that changes the for..in loops to for (i = 0... type loops which are better for iterating over array elements.

dkl
Attachment #631535 - Attachment is obsolete: true
Attachment #636744 - Flags: review?(LpSolit)

Comment 21

5 years ago
Comment on attachment 636744 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v4)

>=== modified file 'Bugzilla.pm'

>-# $::SIG{__DIE__} = i_am_cgi() ? \&CGI::Carp::confess : \&Carp::confess;
>+$::SIG{__DIE__} = i_am_cgi() ? \&CGI::Carp::confess : \&Carp::confess;


>=== modified file 'Bugzilla/Mailer.pm'

>+    return if $email->header('to') eq '';


Could you remove these unrelated changes, please?
Attachment #636744 - Flags: review?(LpSolit)
(Assignee)

Comment 22

5 years ago
Created attachment 636809 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v5)

Oops. Better patch this time.

dkl
Attachment #636744 - Attachment is obsolete: true
Attachment #636809 - Flags: review?(LpSolit)

Comment 23

5 years ago
Comment on attachment 636809 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v5)

>+        for (var i = 0; i < flag_rows.length; i++) {
>+            # Each flag table row should have one flag form select element
>+            # We get the flag type id from the id attribute of the select

Inside JS code, please use // for comments, not #, else the JS code fails. :)
Also, please add a period at the end of sentences.


>+            var show = 0;
>+            # Loop through the allowed flag ids for the selected component
>+            # and if we match, then show the row, otherwise hide the row

Same here.


Otherwise your patch looks good and works fine. r=LpSolit
Attachment #636809 - Flags: review?(LpSolit) → review+

Updated

5 years ago
Flags: approval+
Keywords: relnote
(Assignee)

Comment 24

5 years ago
Created attachment 648043 [details] [diff] [review]
Patch to hide flag rows for when not applicable to component selected (v6)

Sorry about the comments. I think I added the comments in after testing and right before submit to describe what i was doing in the code and failed to realize they were not correct for JavaScript.

Also I do actually hide the flag row if the user cannot set any values in the drop down for the flag. This is the same as before when the flag was just disabled. So no change is needed for that.

dkl
Attachment #636809 - Attachment is obsolete: true
Attachment #648043 - Flags: review?(LpSolit)

Updated

5 years ago
Attachment #648043 - Flags: review?(LpSolit) → review+
(Assignee)

Comment 25

5 years ago
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified template/en/default/bug/create/create.html.tmpl
Committed revision 8323.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 26

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.