1268466 - Can not add CSS rules in iframe

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[lexx92]

Attachment: Example page

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.112 Safari/537.36

Steps to reproduce:

1. Open any page with an iframe.
2. Navigate into any element into iframe.
3. Try to add rule to it with 'add new rule' button with plus sign symbol.


Actual results:

Rule was not added (though style was appended to the end of the page).


Expected results:

Rule with appropriate selector based on element tag/id/class was added.

Comment 1

[Loic]

I guess you speak about the CSS Rules Inspector.

Comment 2

[Patrick Brosset <:pbro>]

Bug triage (filter on CLIMBING SHOES).

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1268466 - ruleview: create new rule <style> el in frame document;r=tromey

To create new rules, style elements are added to the content document.
For nodes located in an iframe, the style element should be added to the
ownerDocument of this particular node.

Review commit: https://reviewboard.mozilla.org/r/49889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49889/

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8747444 [details]
MozReview Request: Bug 1268466 - ruleview: create new rule <style> el in frame document;r=tromey

https://reviewboard.mozilla.org/r/49889/#review46721

I am not 100% sure my issue here makes sense.  Push back if I'm missing something.

::: devtools/server/actors/styles.js:989
(Diff revision 1)
> +   * @param {Document} document
> +   *        The document in which the style element should be appended.
>     * @returns DOMElement of the style tag
>     */
> -  get styleElement() {
> -    if (!this._styleElement) {
> +  getStyleElement: function(document) {
> +    let style = document.getElementById(STYLESHEET_ID);

What if the existing document already has an element of this name?

It seems to me that a safe approach would be to keep a weak map where the keys are documents and the values are style sheets.   Then there would be no need for a magic id.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8747444 [details]
MozReview Request: Bug 1268466 - ruleview: create new rule <style> el in frame document;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49889/diff/1-2/

Comment 6

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/49889/#review46721

> What if the existing document already has an element of this name?
> 
> It seems to me that a safe approach would be to keep a weak map where the keys are documents and the values are style sheets.   Then there would be no need for a magic id.

That was actually what I implemented at first :) 
Then for some reason I thought simply using an ID would be easier and totally overlooked the issue you mentioned.

I reverted back to using weakmaps, hopefully the implementation is ok (not really used to weakmaps).

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8747444 [details]
MozReview Request: Bug 1268466 - ruleview: create new rule <style> el in frame document;r=tromey

https://reviewboard.mozilla.org/r/49889/#review46785

Looks good; ok with one minor nit changed.

::: devtools/server/actors/styles.js:989
(Diff revision 2)
> +   * @param {Document} document
> +   *        The document in which the style element should be appended.
>     * @returns DOMElement of the style tag
>     */
> -  get styleElement() {
> -    if (!this._styleElement) {
> +  getStyleElement: function(document) {
> +    if (!this.styleElements.get(document)) {

I think |.has|, not |.get|, would be clearer here.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8747444 [details]
MozReview Request: Bug 1268466 - ruleview: create new rule <style> el in frame document;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49889/diff/2-3/

Comment 9

[Julian Descottes [:jdescottes]]

Thanks for the review Tom. Addressed the last comment and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a4c3ca79eb .

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9f866b72af4a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f866b72af4a

Comment 12

[Nazir Ahmed Sabbir [:NaSb]]

Managed to reproduce this bug on Nightly 49.0a1 (2016-04-28) (Build ID: 20160428030218) on Linux,

This Bug's Fix is now verified on Latest Firefox Beta 49.0b3

Build ID: 20160811031722
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
OS: Linux 4.4.0-2-deepin-amd64

Comment 13

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 49.0a1 (2016-04-28) on Windows 7 , 64 Bit!

This bug's fix is verified with latest Beta!

 Build ID    20160811031722
 User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160817]