1327767 - Ruleview displays wrong information about inherited rules: reports 1 element instead of many

Status: VERIFIED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url   data:text/html,<div p1>A<div p2>B<a>Color<style>[p1]{color:red}[p2]{color:black}
2. Open inspector -> ruleview, inspect <a> element

AR:  Ruleview shows section "Inherited from div", and shows rules inherited from several div elements
ER:  Ruleview shouldn't lie. Either X or Y
 X) Ruleview should show 2 different sections for each <div> element
 Y) The section should say "Inherited from several div elements"

Comment 1

[Tom Tromey :tromey]

If you change <div p1> to <span p1> you can see that in this case the rule view shows
"Inherited from div" and "Inherited from span", which does seem more clear.

Comment 2

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1327767.patch

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8824151 [details] [diff] [review]
1327767.patch

Sorry Gabriel, but I'm swamped in review/ni requests since I came back from PTO and I need to delegate a few ones if I want to get anything done.

Sorry Sébastien for the delay. Your solution looks good to me. I had investigated this problem too and found something similar.
Thanks for the test too!

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8824151 [details] [diff] [review]
1327767.patch

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

Thanks for this patch Sébastien. This is good to go once we make the changes.

::: devtools/client/inspector/rules/test/browser_rules_inherited-properties_04.js
@@ +5,5 @@
> +"use strict";
> +
> +// Check that inline inherited properties appear in the nested element.
> +
> +const TEST_URI = `<div style="cursor:pointer">A<div style="cursor:pointer">B<a>Cursor`;

We should add the closing brackets for this TEST_URI. It would also be clearer if we change the second div to a span, and then check that it is "Inherited from span".

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Also, we will need to update the commit message to the following:
Bug 1327767 - Show multiples sections for rules inherited from several elements of the same type. r=gl

Comment 6

[Sébastien Blin [:sblin] [:amarok]]

Cool. I will do this tomorrow.

Comment 7

[Sébastien Blin [:sblin] [:amarok]]

Attachment: bug_1327767.patch

I made the changes.

> It would also be clearer if we change the second div to a span, and then check that it is "Inherited from span"
But this patch test the creation of two sections if we have 2 div or 2 span, not one div and one span. So I don't think we need to change this.

Have a nice day,

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8825836 [details] [diff] [review]
bug_1327767.patch

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

Hi Sébastien, 

Thanks for getting to this so quickly. We still also need to update the commit message in your patch file to:
Bug 1327767 - Show multiples sections for rules inherited from several elements of the same type. r=gl

I can also help you push this when it is ready with the changes I mentioned.

::: devtools/client/inspector/rules/test/browser_rules_inherited-properties_04.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Check that inline inherited properties appear in the nested element.

The test comment should also read as follows:
// Tests that different inherited properties sections are created for rules inherited from several elements of the same type.

@@ +10,5 @@
> +                    A
> +                    <div style="cursor:pointer">
> +                    B<a>Cursor</a>
> +                    </div>
> +                  </div>`;

We want to be consistent with how we do multiline strings as the other tests. I would like this to be formatted as the following:
const TEST_URI = `
  <div style="cursor:pointer">
    A
    <div style="cursor:pointer">
      B<a>Cursor</a>
    </div>
  </div>
`;

Comment 9

[Sébastien Blin [:sblin] [:amarok]]

Attachment: bug_1327767.patch

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8825968 [details] [diff] [review]
bug_1327767.patch

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

SHIPIT Sébastien! Did you want me to push this for you?

Comment 11

[Sébastien Blin [:sblin] [:amarok]]

I think I don't have the right to push this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7166b68a598e
Show multiples sections for rules inherited from several elements of the same type. r=gl

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7166b68a598e

Comment 14

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 49.0a1 (2016-05-26) (Build ID: 20160526030223) by the following Comment 0's instruction!

This Bug is now Verified as Fixed on Latest Firefox Beta 53.0b1 (64-bit)

Build ID: 20170307144748
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
OS: Linux 4.8.0-39-generic; Ubuntu 16.04.2 (64 Bit)

Comment 15

[Petruta Horea [:phorea]]

Reproduced this bug with 2017-01-01 53.0a1 build. 
Verified using Firefox 53 beta 1 under Win 10 64-bit and Mac OS X 10.11 that ruleview now shows two different sections for each <div> element.
Marking as verified considering this and comment 14.