Remove inline script / style in browser/devtools/fontinspector/font-inspector.xhtml

RESOLVED FIXED in Firefox 29

Status

RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bgrins, Assigned: k)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Let's move any inline JavaScript, styles, and event handlers out of this file: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/fontinspector/font-inspector.xhtml
(Assignee)

Comment 1

5 years ago
Adding the listener to #showall in font-inspector.js?

Is it normal that showAll() fails in https://mxr.mozilla.org/mozilla-central/source/browser/devtools/fontinspector/font-inspector.js#199 sometimes because node.ownerDocument seems to be not set?
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 2

5 years ago
(In reply to k from comment #1)
> Adding the listener to #showall in font-inspector.js?

Yes, exactly.  this.chromeDoc.querySelector("#showall") should grab the element.  We should also remove the event listener on destroy.  And in init, we should do this.showAll = this.showAll.bind(this) to make sure the context will be correct in the event listener.

> Is it normal that showAll() fails in
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> fontinspector/font-inspector.js#199 sometimes because node.ownerDocument
> seems to be not set?

Not that I know of, but I doubt that changing how the event handler is bound would affect that.  Have you bumped into this problem?
Flags: needinfo?(bgrinstead)
Can you assign me into this bug?
(Reporter)

Comment 4

5 years ago
(In reply to Malintha Fernando from comment #3)
> Can you assign me into this bug?

Malintha, thanks for checking.  Kay, were you already working on this?  If not, then we will assign Malintha to this bug.  Otherwise, I will create a new bug for another file that needs the same change and link to it.
Flags: needinfo?(k)
(Assignee)

Comment 5

5 years ago
Yes, I was working on it, just found this strange behavior with the node.ownerDocument being not set, but I will check this tomorrow.
Flags: needinfo?(k)
(Reporter)

Comment 6

5 years ago
Malintha, take a look at Bug 961531 for a similar one that you can tackle.  If you leave a comment over there I will assign it to you.
Assignee: nobody → k
(Assignee)

Comment 7

5 years ago
I get this error when clicking on "See all fonts used in this page":

JavaScript error: chrome://browser/content/devtools/fontinspector/font-inspector.js, line 199: contentDocument is undefined

Tried on google.de and facebook.com
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 8

5 years ago
(In reply to k from comment #7)
> I get this error when clicking on "See all fonts used in this page":
> 
> JavaScript error:
> chrome://browser/content/devtools/fontinspector/font-inspector.js, line 199:
> contentDocument is undefined
> 
> Tried on google.de and facebook.com

Yes, I don't think this is related to your changes (I'm seeing it with no patches applied).  I've filed Bug 962085 to fix this.

I'd say you can proceed on this bug without worrying about this error.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 9

5 years ago
Created attachment 8362993 [details] [diff] [review]
bug-960737.patch

Okay.
Attachment #8362993 - Flags: review?(bgrinstead)
(Reporter)

Comment 10

5 years ago
Comment on attachment 8362993 [details] [diff] [review]
bug-960737.patch

Review of attachment 8362993 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/fontinspector/font-inspector.js
@@ +207,5 @@
>  
>  window.setPanel = function(panel) {
>    window.fontInspector = new FontInspector(panel, window);
> +
> +  let button = document.getElementById("showall");

Can you move this logic into FontInspector.init and remove the handler in FontInspector.destroy?  This way it will be more consistent with the way the some of the other sidebar tabs get opened and closed.

In order to get this to work, you could set a couple of extra properties on init:

this.showAll = this.showAll.bind(this);
this.showAllButton = this.chromeDoc.getElementById("showall");
Attachment #8362993 - Flags: review?(bgrinstead)
(Assignee)

Comment 11

5 years ago
Created attachment 8363614 [details] [diff] [review]
clearedHtmlFromCssAndJs.patch

I always solved those |this| problems with closures. The solution with bind seems much better.

thanks for the tip :)
Attachment #8362993 - Attachment is obsolete: true
Attachment #8363614 - Flags: review?(bgrinstead)
(Reporter)

Comment 12

5 years ago
Comment on attachment 8363614 [details] [diff] [review]
clearedHtmlFromCssAndJs.patch

Review of attachment 8363614 [details] [diff] [review]:
-----------------------------------------------------------------

Code changes look good to me, I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1acbde985a24
Attachment #8363614 - Flags: review?(bgrinstead) → review+
(Reporter)

Comment 13

5 years ago
(In reply to k from comment #11)
> Created attachment 8363614 [details] [diff] [review]
> clearedHtmlFromCssAndJs.patch
> 
> I always solved those |this| problems with closures. The solution with bind
> seems much better.
> 
> thanks for the tip :)

No problem, and thank you for taking on these bugs!
(Reporter)

Comment 14

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/081e3e5c6d0a
https://tbpl.mozilla.org/?tree=Fx-Team&rev=081e3e5c6d0a
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/081e3e5c6d0a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=bgrins][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 29

Updated

5 years ago
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][qa-]

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.