Closed Bug 493020 Opened 16 years ago Closed 16 years ago

The side-by-side diff in pages history is not properly aligned

Categories

(support.mozilla.org :: Knowledge Base Software, task, P1)

Product:
support.mozilla.org
Component:
Knowledge Base Software
Type:
task

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: underpass_bugzilla, Assigned: ecooper)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: sumo_only)

Attachments

(1 file)

v1
339 bytes, patch
paulc
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; it; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10 Build Identifier: Until a few days ago, when invoking the side-by-side diff of two different versions of a page, the text was equally alligned in the two columns. Now this seems to be broken, and it's difficult to track the changes in a page. Reproducible: Sometimes Actual Results: Example: https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Keyboard+shortcuts&compare=1&oldver=31&newver=32&diff_style=sidediff-full Expected Results: Oddly enough, some pages still show the correct behaviour: https://support.mozilla.com/tiki-pagehistory.php?page=%2ANo+sound+in+Firefox&compare=1&oldver=11&newver=12&diff_style=sidediff-full
Summary: The side-by-side diff is not properly alligned → The side-by-side diff in pages history is not properly alligned
Simone, is this one aligned right? https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=*Keyboard+shortcuts&compare=1&oldver=31&newver=32&diff_style=sidediff-full This is the staging version of the first link you provided. Maybe you can provide a few more examples so we can see if there's a pattern?
Eric, do you have time to look at this? Laura? Simone mentions this bug is very important :) for updating the article v3.5
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is this a regression or has it always been this bad? This looks pretty horrible and I'm worried about the 3.5 release which is quickly approaching. SUMO 1.2 is scheduled for a June 11 release. I'd love to see a patch for this asap and then we can figure out how to best get this out so localizers can start to update documentation for 3.5 without having to go through the pain Simone and friends did.
It's a regression.
Do we know when it regressed?
I think it worked well until 05-12-2009 (approx.)
Looking at list of the fixed bugs in the 1.0.2 release, I found bug 450342
Ah, that definitely looks like the cause of this. Good catch Simone! Eric, can you take a look at this?
Assignee: nobody → smirkingsisyphus
Target Milestone: --- → 1.2
Keywords: regression
Whiteboard: wanted in 1.1.1
Whiteboard: wanted in 1.1.1
Target Milestone: 1.2 → 1.1.1
(In reply to comment #10) > Ah, that definitely looks like the cause of this. Good catch Simone! Eric, can > you take a look at this? Yeah, it seems line wrapping isn't something that meshes well with a lot of the diff styles. I'd say the approach would be: 1) back out r24453/r24454 from bug 450432 as a solution for this bug 2) move this to 1.1 3) reopen bug 450432 with a reference to this one and decide on a better approach 4) move bug 450432 to 1.1.1 I'd much rather have this fixed and deal with bug 450432 in the interim. Sound reasonable?
Priority: -- → P1
ecooper: if I can vote, I say "Aye" to your solution. :)
(In reply to comment #12) > ecooper: if I can vote, I say "Aye" to your solution. :) David what's your take? Do you want to go this route (comment 11)?
Eric, what does bug 450432 have to do with this problem? I don't really follow, I'm afraid. The reason why we need to fix the diff issue in 1.1.1 is because we want the fix to be available _before_ Firefox 3.5 is released. If your proposed approach in comment 11 aims to still do that while not regressing other things, I am fine with the approach (but still don't quite understand it).
(In reply to comment #14) > Eric, what does bug 450432 have to do with this problem? I don't really follow, > I'm afraid. > > The reason why we need to fix the diff issue in 1.1.1 is because we want the > fix to be available _before_ Firefox 3.5 is released. If your proposed approach > in comment 11 aims to still do that while not regressing other things, I am > fine with the approach (but still don't quite understand it). I'm dumb. Bug 450342. It'll make sense with that one.
I still don't really understand the proposed shuffling with the bugs. I looked at the screenshot of how it looked like before the other bug was fixed (attachment 333491 [details]) and it looks quite horrible. Are you suggesting we go back to that state?
(In reply to comment #16) > I still don't really understand the proposed shuffling with the bugs. I looked > at the screenshot of how it looked like before the other bug was fixed > (attachment 333491 [details]) and it looks quite horrible. Are you suggesting we go back > to that state? I have a trivial fix (one-line CSS change) that I'll apply when I get settled in for today's meeting that seems to take care of this without the hassle of what I said in comment 11.
Please submit a patch and have Laura or Paul review!
Attached patch v1 — — Splinter Review
It seems you need to use min-width as it won't render properly with just width. Anyway, very trivial fix that seems to take care of the problem. I think.
Attachment #379703 - Flags: review?(paul.craciunoiu)
Comment on attachment 379703 [details] [diff] [review] v1 I tested this on: https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Keyboard+shortcuts&compare=1&oldver=31&newver=32&diff_style=sidediff-full (my local URL of course) And about 2 others. Looks good!
Attachment #379703 - Flags: review?(paul.craciunoiu) → review+
Sorry, Paul, did you try this on support-stage or even on the production site? The link you provided I see just like before: not well formatted. Platforms tested: Mozilla/5.0 (X11; U; Linux i686; it; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10 Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10
Simone, Paul was just reviewing Eric's patch, but it hasn't been committed to stage yet. Eric (or Paul) will do it shortly though. :)
(In reply to comment #20) > (From update of attachment 379703 [details] [diff] [review]) > I tested this on: > https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Keyboard+shortcuts&compare=1&oldver=31&newver=32&diff_style=sidediff-full > (my local URL of course) > And about 2 others. Looks good! I should say, Eric, since we have both min- and max-width equal (and IE6 doesn't support these AFAIK), why not just replace both with a fixed with?
(In reply to comment #20) > I tested this on: [snip] > And about 2 others. Looks good! Sorry :-[ I read "tested" and I did not understand... I will not post other comments... Thanks :-)
(In reply to comment #23) > (In reply to comment #20) > > (From update of attachment 379703 [details] [diff] [review] [details]) > > I tested this on: > > https://support.mozilla.com/tiki-pagehistory.php?locale=en-US&page=Keyboard+shortcuts&compare=1&oldver=31&newver=32&diff_style=sidediff-full > > (my local URL of course) > > And about 2 others. Looks good! > > I should say, Eric, since we have both min- and max-width equal (and IE6 > doesn't support these AFAIK), why not just replace both with a fixed with? It renders slightly differently (vs. using just width). I'm not sure which looks better, of course.
Can we get this checked in so Simone and others can test please?
r26388/r26389 Stage will be updated with these changes when the minify cache refreshes. Please give it about two hours to let it purge itself.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I'm joining the positive choir (well, only duo so far): it's looking a lot better! :)
We are a trio, then. The text is now well alligned (tried with several page as samples, will try with some more later) :) The only strange thing I still see is something probably related to the character encoding in the page, since the letters à è etc. are sometimes showed with as �
Thanks, Simone -- I'm going to verify this, and if we see any regressions, we can file them anew.
Status: RESOLVED → VERIFIED
Summary: The side-by-side diff in pages history is not properly alligned → The side-by-side diff in pages history is not properly aligned
Target Milestone: 1.1.1 → 1.1
Whiteboard: sumo_only
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: