Last Comment Bug 726392 - Text with "-moz-text-align-last: right" does not hug the right as the window widens
: Text with "-moz-text-align-last: right" does not hug the right as the window ...
: testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Simon Montagu :smontagu
Depends on: 729759
Blocks: randomstyles refdyn 536557
  Show dependency treegraph
Reported: 2012-02-11 20:02 PST by Jesse Ruderman
Modified: 2012-02-22 16:22 PST (History)
2 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (94 bytes, text/html)
2012-02-11 20:02 PST, Jesse Ruderman
no flags Details
Patch (3.15 KB, patch)
2012-02-13 21:47 PST, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch (4.56 KB, patch)
2012-02-13 21:48 PST, Simon Montagu :smontagu
dbaron: review+
Details | Diff | Splinter Review
Reftests (17.60 KB, patch)
2012-02-13 21:48 PST, Simon Montagu :smontagu
dbaron: review-
Details | Diff | Splinter Review
Patch v.2 (4.84 KB, patch)
2012-02-16 09:02 PST, Simon Montagu :smontagu
dbaron: review+
Details | Diff | Splinter Review
Reftests v.2 (17.59 KB, patch)
2012-02-16 09:06 PST, Simon Montagu :smontagu
dbaron: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-02-11 20:02:44 PST
Created attachment 596416 [details]

1. Load the testcase.
2. Widen the window.

Result: The text stays where it is, so it's no longer aligned to the right.

Expected: The text should move in order to stay at the right side of the window.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2012-02-11 21:15:06 PST
nsBlockFrame::PrepareResizeReflow needs to be adjusted for text-align-last, probably?
Comment 2 Simon Montagu :smontagu 2012-02-13 21:47:03 PST
Created attachment 596920 [details] [diff] [review]
Comment 3 Simon Montagu :smontagu 2012-02-13 21:48:21 PST
Created attachment 596921 [details] [diff] [review]
Comment 4 Simon Montagu :smontagu 2012-02-13 21:48:54 PST
Created attachment 596923 [details] [diff] [review]
Comment 5 David Baron :dbaron: ⌚️UTC-10 2012-02-15 16:53:11 PST
Comment on attachment 596921 [details] [diff] [review]

+bool static inline IsAlignedLeft(const PRUint8 aAlignment,
+                                 const PRUint8 aDirection,
+                                 const PRUint8 aUnicodeBidi)
+  return (NS_STYLE_TEXT_ALIGN_LEFT == aAlignment ||
+          (NS_STYLE_TEXT_ALIGN_DEFAULT == aAlignment &&
+           NS_STYLE_DIRECTION_LTR == aDirection &&
+           !(NS_STYLE_UNICODE_BIDI_PLAINTEXT & aUnicodeBidi)) ||
+          (NS_STYLE_TEXT_ALIGN_END == aAlignment &&
+           NS_STYLE_DIRECTION_RTL == aDirection));

Two issues here:

 (1) You probably need to check unicode-bidi: plaintext for the
     rtl + end case as well, no?  (Can we have a test for that too?)
     (Yes, you're just moving an existing bug.)

 (2) You should probably add a comment above the function that it's
     a test for whether lines are *certain* to be aligned left so that
     we can make resizing optimizations.

In PrepareResizeReflow:
 * put the result of GetStyleTextReset() in a variable at the top
   (for the 2 uses)
 * inside the interesting for loop, put |isLastLine| in a variable
   since it can be used twice (once negated) and allow removing a comment
   "not the last line"

r=dbaron with that
Comment 6 David Baron :dbaron: ⌚️UTC-10 2012-02-15 17:55:23 PST
Comment on attachment 596923 [details] [diff] [review]

The tests look fine... except that I'm really curious why they're fuzzy?  What part is fuzzy, and is it fixable?  I really wouldn't have expected that... and I don't think you should need it in this case.

(I might be open to changing to review+ if there's a good reason.)
Comment 7 Simon Montagu :smontagu 2012-02-16 09:02:56 PST
Created attachment 597849 [details] [diff] [review]
Patch v.2

You might want to give this another quick look: As well as addressing your comments I've added a condition !line->IsLineWrapped() for the last line test in the for loop to catch lines that are not last lines but are followed by a forced line break, so text-align-last applies to them.
Comment 8 Simon Montagu :smontagu 2012-02-16 09:06:16 PST
Created attachment 597852 [details] [diff] [review]
Reftests v.2

Adding letter-spacing makes the reftests pass without fuzz.
Comment 9 Simon Montagu :smontagu 2012-02-16 09:13:24 PST
I still don't really know why the reftests needed fuzz before: they were showing differences in anti-aliasing pixels between the letters, similar to what we get when the test renders text in separate operations and the reftest renders it in one go, e.g. bug 696671 and several others. Nothing like that happens here though, my only guess is that it might be some kind of effect from rounding when calculating 50% of the width.
Comment 10 David Baron :dbaron: ⌚️UTC-10 2012-02-17 16:26:56 PST
Comment on attachment 597852 [details] [diff] [review]
Reftests v.2

Well, r=dbaron, I guess, though the fuzz issue might be worth investigating sometime.
Comment 11 David Baron :dbaron: ⌚️UTC-10 2012-02-17 16:30:26 PST
Comment on attachment 597849 [details] [diff] [review]
Patch v.2

r=dbaron.  Might also be worth testing the forced-break case.  And don't forget to remove the try syntax from the commit message.

Note You need to log in before you can comment on or make changes to this bug.