Closed Bug 1004499 Opened 6 years ago Closed 6 years ago

drawFocusIfNeeded draws unconditionally


(Core :: Canvas: 2D, defect)

Not set





(Reporter: enndeakin, Assigned: cabanier)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

The spec says to only draw the focus ring when one should be drawn on the element passed in. Yet, the current code only checks if it is focused.

The check should also be checking if nsPIDOMWindow::ShouldShowFocusRing() returns true.
Assignee: nobody → cabanier
Attachment #8415979 - Flags: review?(enndeakin)
I'm trying to push a try build but the servers are stuck again.
Blocks: 952643
No longer blocks: 952643
Blocks: 935992
Comment on attachment 8415979 [details] [diff] [review]
Added code to check if the window should draw a focus ring.

>   nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>-  if (fm) {
>+  if (fm && mCanvasElement) {
>     // check that the element i focused
>     nsCOMPtr<nsIDOMElement> focusedElement;
>     fm->GetFocusedElement(getter_AddRefs(focusedElement));
>     if (SameCOMIdentity(aElement.AsDOMNode(), focusedElement)) {
>-      return true;
>+      nsPIDOMWindow *window = mCanvasElement->OwnerDoc()->GetWindow();
>+      if (window) {
>+        return window->ShouldShowFocusRing();
>+      }

I know that you've checked that aElement is a descendant of the canvas, but it would be clearer if you used aElement->OwnerDoc()->GetWindow() here, and remove the null check earlier.

You may find that some tests will now fail as the ring is no longer being shown unconditionally. You can simulate a keypress if needed.
Attachment #8415979 - Flags: review?(enndeakin) → review+
Attachment #8415979 - Attachment is obsolete: true
Keywords: checkin-needed
sorry had to backout this change for causing bustage like
mozilla::dom::Element& aElement. Probably should be aElement.OwnerDoc()?
Attachment #8416018 - Attachment is obsolete: true
(In reply to Nochum Sossonko [:Natch] from comment #8)
> mozilla::dom::Element& aElement. Probably should be aElement.OwnerDoc()?

yes. sorry about that! Was trying to be too quick
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.