Closed Bug 652410 Opened 9 years ago Closed 9 years ago

500+ consecutive lines of markup whitespace in show_bug.cgi flags table, depending on flag states

Categories

(Bugzilla :: User Interface, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: emorley, Assigned: dkl)

References

Details

(Whiteboard: [bmo4.0-resolved])

Attachments

(1 file, 1 obsolete file)

On certain bugs, show_bugs.cgi generates hundreds of consecutive lines of whitespace in the flags table. Some bugs seem worse than others - I believe due to what component they are in (and as such how many flags are shown) and also the state of those flags.

I presume one of the [% FOR ... %] blocks has too many blank lines outside of the [% ... %] in the template.

1) View source of https://bugzilla.mozilla.org/show_bug.cgi?id=648866
2) Scroll down to <table id="flags">

Expected:
As few blank lines of whitespace as possible (eg max one or two between each non-whitespace block).

Actual:
~250 lines of whitespace, followed by a single flag <tr> block and then ~250 more lines of whitespace.
Confirmed. There are a few lines of whitespace using stock 4.0 on landfill.bugzilla.org but nowhere near the amount that BMO is adding. Looking into it.

dkl
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Component: Bugzilla: Other b.m.o Issues → User Interface
Product: mozilla.org → bugzilla.mozilla.org
QA Contact: general → ui
Version: other → Current
Basically the looping code in the flag list template inserts a lot of extra newlines since there are quite a few 'inactive' flags for the particular Product/Component combination. This patch will suppress the extra newlines so that the source looks more condensed when only a few flags from a large list are actually active.

Please review
dkl
Attachment #531360 - Flags: review?(glob)
Comment on attachment 531360 [details] [diff] [review]
Patch to remove extra newlines in flag list html (v1)

r=glob looks fine
Attachment #531360 - Flags: review?(glob) → review+
Pushing upstream.
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: ui → default-qa
Version: Current → 4.0
Comment on attachment 531360 [details] [diff] [review]
Patch to remove extra newlines in flag list html (v1)

I want to give it a look before approving this patch. It seems you are removing several newlines which will make the code harder to read. Readability is more important than some extra newlines.
Attachment #531360 - Flags: review?(LpSolit)
Target Milestone: --- → Bugzilla 4.0
Whiteboard: [bmo4.0-resolved]
(In reply to comment #5)
> Readability is more important than some extra newlines.

Yeah but 500 newlines?
(In reply to comment #6)
> Yeah but 500 newlines?

I'm not talking about the output, but the code itself.
Comment on attachment 531360 [details] [diff] [review]
Patch to remove extra newlines in flag list html (v1)

>+  [%- FOREACH type = flag_types -%]
>     [%# Step 1a: Display existing flag(s). %]
>+    [%- FOREACH flag = type.flags -%]

Are all these [%- -%] really needed? Do they really help removing newlines?


>-    [% END %]
>-    
>+    [%- END -%]
>     [%# Step 1b: Display UI for setting flag. %]

Removing the newline here makes the template really hard to parse, IMO.


>-  [% END %]
>-
>+  [%- END -%]
>   [%# Step 2: Display flag type again (if type is multiplicable). %]

Same here.


>     [% END %]
>-
>     [% PROCESS flag_row first_cell_empty = 0 addl_text = "addl." %]

Same here.


Please tell me if these removed newlines are really required. I have the feeling that they are not. In the case I'm wrong, please re-request review from me.
Attachment #531360 - Flags: review?(LpSolit) → review-
Here is a much more scaled down version of the patch. After doing some more experimentation, I was able to figure out where to collapse whitespace to bring down the line count significantly. And without any loss of readability on the template itself. 

dkl
Attachment #531360 - Attachment is obsolete: true
Attachment #542072 - Flags: review?(LpSolit)
Comment on attachment 542072 [details] [diff] [review]
Patch to remove extra newlines in flag list html (v2)

Yes, much better. Thanks! r=LpSolit
Attachment #542072 - Flags: review?(LpSolit) → review+
Flags: approval4.0+
Flags: approval+
Thanks

trunk:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified template/en/default/flag/list.html.tmpl
Committed revision 7852.

4.0:
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.0
modified template/en/default/flag/list.html.tmpl
Committed revision 7613.

dkl
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.