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)
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)
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.
Comment 2•7 years ago
|
||
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
Comment 3•7 years ago
|
||
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 :)
Comment 4•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
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! :)
Comment 9•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
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 16•7 years ago
|
||
mozreview-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+
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcb14bed5eb619f8c37259ba6028b380c35b64f0
Updated•7 years ago
|
Attachment #8901715 -
Flags: review?(gl)
Updated•7 years ago
|
Attachment #8903311 -
Flags: review?(gl)
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
Thank you for both of your contribution Christopher and Thomas!
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f63826a2f12b https://hg.mozilla.org/mozilla-central/rev/fcd3a7c6694c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 21•7 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•