Closed Bug 1327767 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

(firefox53 verified)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: arni2033, Assigned: sblin)

References

Details

Attachments

(1 file, 2 obsolete files)

>>>   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"
Component: Untriaged → Developer Tools: Inspector
Priority: -- → P2
No longer blocks: 1277113
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
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.
Attached patch 1327767.patch (obsolete) — Splinter Review
Attachment #8824151 - Flags: review?(pbrosset)
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!
Attachment #8824151 - Flags: review?(pbrosset) → review?(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".
Attachment #8824151 - Flags: review?(gl) → review+
Assignee: nobody → amarok
Status: NEW → ASSIGNED
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
Cool. I will do this tomorrow.
Attached patch bug_1327767.patch (obsolete) — Splinter Review
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,
Attachment #8824151 - Attachment is obsolete: true
Attachment #8825836 - Flags: review?(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>
`;
Attachment #8825836 - Flags: review?(gl) → review+
Attachment #8825836 - Attachment is obsolete: true
Attachment #8825968 - Flags: review?(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?
Attachment #8825968 - Flags: review?(gl) → review+
I think I don't have the right to push this.
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7166b68a598e
Show multiples sections for rules inherited from several elements of the same type. r=gl
https://hg.mozilla.org/mozilla-central/rev/7166b68a598e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Flags: qe-verify+
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)
QA Whiteboard: [bugday-20170308]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.