239785 - only first part blinks for elements split into multiple lines

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Michal Ceresna]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040317 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040317 Firefox/0.8

open the e.g. the about: page,
resize the window so, that e.g. the "Mozilla Public License" link is
splitted into 2 lines
then with 'select element by click' select the splitted link.
only first part of it will blink

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Michal Ceresna]

Attachment: proposed fix

Comment 2

[Alex Vincent [:WeirdAl]]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040405
OS -> All, confirmed

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 145539 [details] [diff] [review]
proposed fix

Reporter: have you tested this patch?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 145539 [details] [diff] [review]
proposed fix

Looks very good!! Thanks

I think you can move the rendering context creation above the loop. Also
setting p2t can be outside the loop.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Michal, if you can revise the patch as suggested, and post the new patch I will
be glad to give you sr+. I'll even hunt down caillon and get him to review it :-)

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 145539 [details] [diff] [review]
proposed fix

Roc: no hunting necessary.  I may be moving, but I'm still sort of around,
although my response time will not be as instantaneous as I wish it to be.  ;-)

Michal: What roc said.  Additionally, I have nits.  Do not differ your naming
style of arguments to DrawOutline (all arguments should be named in the fashion
of 'aInterCaps').

Also in the implementation of DrawOutline, please follow the one-line-if style of

  if (foo) {
    bar;
  }

And if you could please add spaces in between operator+ and its arguments on the
lines you modified in DrawOutline, as well as between operator== on the line you
declare isLastFrame, I would be very grateful.


>+    rect.x = origin.x;
>+    rect.y = origin.y;

The above can change to:

     rect.MoveTo(origin);


I'll have another look at a new patch.  (Bonus points: a diff -w would be nice)

Comment 7

[Michal Ceresna]

Attachment: revised wrt. comments

double checked, trailing whitespace is trimmed

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 145678 [details] [diff] [review]
revised wrt. comments

Looks great!

+  nsCOMPtr<nsIRenderingContext> rcontext = nsnull;

The = nsnull is not necessary, nsCOMPtrs are initialized to null by default.
Feel free to fix this before you check in.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 145678 [details] [diff] [review]
revised wrt. comments

>+  PRBool isFirstFrame = PR_TRUE;
>+  nsCOMPtr<nsIRenderingContext> rcontext = nsnull;

No need to initialize to nsnull here.


>+  nsIFrame* frame = inLayoutUtils::GetFrameFor(aElement, presShell);
>+  while (frame) {
>+    if (!rcontext) {
>+      presShell->CreateRenderingContext(frame, getter_AddRefs(rcontext));
>+    }
>+    // get view bounds in case this frame is being scrolled
>+    nsRect rect = frame->GetRect();
>+    nsPoint origin = inLayoutUtils::GetClientOrigin(presContext, frame);
>+    rect.MoveTo(origin);
>+    mCSSUtils->AdjustRectForMargins(frame, rect);
>+
>+    if (mInvert) {
>+      rcontext->InvertRect(rect.x, rect.y, rect.width, rect.height);

rcontext->InvertRect(rect);

(sorry I didn't catch this the first time)

>+    }
>+
>+    frame->GetNextInFlow(&frame);
>+
>+    PRBool isLastFrame = (frame==nsnull);

spaces, please, a la:

  PRBool isLastFrame = (frame == nsnull);


r=caillon with nits addressed.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is good to go. Michal, if you attach the final patch, I can check it in for
you.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Michal, thanks for the fix.  Checked into trunk 2004-05-18 10:52 PDT.