Page MenuHomePhabricator

Broken diff header layout for multi diffs with no changes
Closed, ResolvedPublic

Description

The FlaggedRevs extension display two diffs in one table row instead of two rows if you’re looking at the diff between two version and there is no difference (e. g. if you reverted some vandalism).


Version: 1.16.x
Severity: trivial
URL: http://de.wikipedia.org/w/index.php?title=Hans_Joachim_Sch%C3%A4dlich&action=historysubmit&diff=65352667&oldid=61888270

Details

Reference
bz21053

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:51 PM
bzimport set Reference to bz21053.

Created attachment 6649
sets width of the cols to 50%

The problem is that there are two tables: the actual diff table and the FlaggedRev’s table above. The width of the FR table is 100% and the width of their columns is 50%. The width of the diff table is fixed to (100%), too, but the width of their columns is not fixed. So if the columns have a different width, this bug occurs.

So in this proposed patch, I added width:50%; to the columns.

attachment diff.patch ignored as obsolete

You can easily reproduce this on en.wikipedia, (e.g http://en.wikipedia.org/w/index.php?title=Rumson,_New_Jersey&action=historysubmit&diff=317019545&oldid=317003264) so I don't think this has something to do with FlaggedRevs.

My guess is that the colspan="4" attribute of the multi diff notice is causing this.

Yes, you’re rigth. (In reply to comment #3)

You can easily reproduce this on en.wikipedia, (e.g
http://en.wikipedia.org/w/index.php?title=Rumson,_New_Jersey&action=historysubmit&diff=317019545&oldid=317003264)
so I don't think this has something to do with FlaggedRevs.

My guess is that the colspan="4" attribute of the multi diff notice is causing
this.

It was just my first thought that the bug may lie in the FlaggedRevs extension, but this is wrong, as you say. Nevertheless, my proposed patch fixes (or should fix) your example, too.

(In reply to comment #4)

It was just my first thought that the bug may lie in the FlaggedRevs extension,
but this is wrong, as you say. Nevertheless, my proposed patch fixes (or should
fix) your example, too.

Honestly, I think it's better to fix the layout than trying to override it with some css styles. The multi diff notice has a colspan of 4 whereas the rest of the table has only two columns.

Ok. Would it bee all right to show the four <col>s everytime and add a colspan="2" to the header <td>s? This would fix the problem, too.

(In reply to comment #6)

Ok. Would it bee all right to show the four <col>s everytime and add a
colspan="2" to the header <td>s? This would fix the problem, too.

Apparently it had been this way until r49688, where it was changed to fix bug 18538. So better change the colspan of the multi diff notice to 2 if no diff is shown.

Created attachment 6651
Change colspan

The error was that in the mentioned revision no one paid attention to the colspan of the diff-multi thing. So I added a $colspanHeader variable which deals with the problem.

Attached:

  • Bug 21066 has been marked as a duplicate of this bug. ***

rockmfr wrote:

The cause of the visible output change was actually r56406. Prior to that, the "if( $diff )" check would always evaluate to true because the debug output would always be present. So, that whole check is totally fubar. It should really be checking to see if something like "<tr>" is present. Though, that is a bit of a hack. Is there a way to do this without changing the colspans at all?

Interesting, Brion reported bug 18538 fixed in r49688 using "current safari/mac" on 28 April 2009, even though the change in r49688 was ineffectual until r56406. I've also just confirmed that diff output from r56405 (which I confirm seems to not be showing any effect due to r49688) displays correctly in Safari/Mac 4.0.3. Maybe r49688 can just be reverted, if it's only old versions of Safari that have the problem? Has anyone tested a recent version of Chrome?

  • Bug 21433 has been marked as a duplicate of this bug. ***

*** Bug 13875 has been marked as a duplicate of this bug. ***