Closed Bug 1085119 Opened 10 years ago Closed 7 years ago

Computed style reported as fixed but style rule says absolute

Categories

(DevTools :: Inspector, defect, P3)

56 Branch
defect

Tracking

(firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: kats, Assigned: judahiii, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(3 files, 1 obsolete file)

Attached image devtools.png
See attached screenshot. Maybe I'm just missing something but I'm not sure why the computed "position" style is getting reported as "fixed" when the active style rule says "absolute". Are there other factors that come into play here that aren't reported by devtools?

For reference, this is the style on the iframe that's full-screened when watching a video in the youtube app on B2G (following STR in bug 1076783 comment 15). WebIDE was in a recent desktop nightly on OS X.
Bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
This issue was also mentioned on Stack Overflow[1]. An answer to it[2] indicates that this is related to minified sources.
It also includes a minimal test case:

http://jsfiddle.net/oy4rkyax/

As the Rules View doesn't have this issue, I assume the fix for this bug shouldn't be too difficult, therefore I mark it as 'good-first-bug'.

Sebastian

[1] https://stackoverflow.com/q/31193714/432681
[2] https://stackoverflow.com/a/31193903/432681
Keywords: good-first-bug
OS: Mac OS X → All
Hardware: x86 → All
Version: Trunk → 56 Branch
This bug seems to have been solved since last month since I can't reproduce it in neither my 08-08-2017 nor my 24-08-2017 nightly builds.

Tell me if I'm wrong, that's my first message on BugZilla :)
I've tested it on Nightly 57.0a1 (2017-08-25) right now and I can still reproduce the bug.

Here are the steps to reproduce the test case from comment 2:
1. Open http://jsfiddle.net/oy4rkyax/
2. Inspect the "Hello world" in the output frame
3. Switch to the Computed side panel
4. Expand the "color" property

=> You'll see "red" standing at the top of the list, then "blue", then "green", both crossed out.
The list should actually be the other way round, i.e. "green" at the top and not crossed out and red at the bottom crossed out.

As mentioned earlier, the issue is caused by the CSS rules being placed at the same line. If they are placed on different lines, the order is correct. So, not only the line of the rule needs to be taken into account but also the column.

Sebastian
Unfortunately (!) I didn't notice Thomas Lacroix's patch. I took a different approach and, rather than taking the last of the "best matched bugs," I consider column when deciding best rule. I think this is possibly a better approach because it's more readable and doesn't rely on the array being populated in order from beginning to end of document, but I do not really know.

I don't know what to do with having two patches as I'm new to Mozilla contributions.
Yes indeed I too think that your approach is better than mine, because I'm not sure either if we can rely on the array's order, and mine imposes to have a stable sort. And I must admit that yours is more elegant! :)
I agree that the approach considering the column is a bit better. Sorry Thomas!
I've set Gabriel as reviewer for Christopher's patch, so this gets some attention. Gabriel, feel free to forward the request to someone else if you're not the right one for this or don't have time to do the review.

Christopher, maybe you could also add a unit test to ensure that this doesn't break again in the future? You can find the other tests for the Computed panel at https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/test.

Sebastian
Assignee: nobody → judahiii
Mentor: sebastianzartner, gl
Status: NEW → ASSIGNED
I originally made a mistake on `hg push review`, replacing the initial commit with the unit test instead of adding it. I think I've fixed it now though.
Comment on attachment 8901578 [details]
Bug 1085119 - Fixed wrong computed best matched style rule in DevTools

I think this is obsolete now.
Attachment #8901578 - Attachment is obsolete: true
Comment on attachment 8903311 [details]
Bug 1085119 - Add unit tests for css-logic row and column precedence

https://reviewboard.mozilla.org/r/175104/#review181990

Thank you for the patches.  These seem reasonable to me.

Can you run them through try?  If not, let me know, and I will do it.
Attachment #8903311 - Flags: review+
Comment on attachment 8901715 [details]
Bug 1085119 - Make devtools css-logic consider column when choosing best rule

https://reviewboard.mozilla.org/r/173150/#review181992

Thanks, this looks good.
Attachment #8901715 - Flags: review+
Attachment #8901715 - Flags: review?(gl)
Attachment #8903311 - Flags: review?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f63826a2f12b
Part 1: Make devtools css-logic consider column when choosing best rule r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd3a7c6694c
Part 2: Add unit tests for css-logic row and column precedence r=tromey
Thank you for both of your contribution Christopher and Thomas!
https://hg.mozilla.org/mozilla-central/rev/f63826a2f12b
https://hg.mozilla.org/mozilla-central/rev/fcd3a7c6694c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reproduced in 57.0a1 2017-09-01, fixed in 57.0a1 2017-09-12. Thanks a lot for fixing this, Christopher!

Sebastian
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.