Bug 28973 – REGRESSION: No margin shorthands showing

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Timothy Hatcher]

No margin shorthand is showing, when they should.

Steps:
1) Visit webkit.org
2) Inspect #icon
3) Look at the styles.

Results:
No margin shorthand showing.

Expected:
A margin shorthand of 0px.

[Timothy Hatcher]

Created attachment 39068 [details]
Bug

[Timothy Hatcher]

Created attachment 39069 [details]
Expected

[Yury Semikhatsky]

Created attachment 39265 [details]
test page

This regression was introduced with the fix for https://bugs.webkit.org/show_bug.cgi?id=28635. I've attached a page where where margins shorthand is empty. If 'margin: 0px;' is moved to the top of the #icon style declaration block the margin shorthand becomes 0px in the Web Inspector.

[Yury Semikhatsky]

Created attachment 39266 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 39266 [details]
patch

We can write a LayoutTest for this, no?

[Yury Semikhatsky]

Created attachment 39273 [details]
patch with layout test

[Timothy Hatcher]

Comment on attachment 39273 [details]
patch with layout test

You need to fill in the info in the LayoutTests ChangeLog.

[Yury Semikhatsky]

Created attachment 39274 [details]
patch with layout test

Added LayoutTest/ChangeLog entry.

[Yury Semikhatsky]

(In reply to comment #7)
> (From update of attachment 39273 [details])
> You need to fill in the info in the LayoutTests ChangeLog.

Done.

[Eric Seidel (no email)]

Comment on attachment 39274 [details]
patch with layout test

Tests are better when they contain the expected results in them.  In this case, I don't know if your test results are correct or not, and new implementors woudl have to compare to the existing -expected.txt files to know if their implementations are failing or passing this.

javascript-only tests are also preferred in cases like this:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

Numerous examples of js-only tests in resources/ directories containing a TEMPLATE.html file.

This is OK, but the test definitely could be slicker. :)

[Yury Semikhatsky]

Created attachment 39328 [details]
patch with pure js layout test

[Yury Semikhatsky]

(In reply to comment #10)
> (From update of attachment 39274 [details])
> Tests are better when they contain the expected results in them.  In this case,
> I don't know if your test results are correct or not, and new implementors
> woudl have to compare to the existing -expected.txt files to know if their
> implementations are failing or passing this.
> 
> javascript-only tests are also preferred in cases like this:
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
> 
> Numerous examples of js-only tests in resources/ directories containing a
> TEMPLATE.html file.
> 
> This is OK, but the test definitely could be slicker. :)

Thanks for your suggestion. I'm new to the layout tests and it's not obvious which of them can be considered as reference implementation. I refactored the test so that it uses TEMPLATE.html.

[WebKit Commit Bot]

Comment on attachment 39328 [details]
patch with pure js layout test

Rejecting patch 39328 from commit-queue.

('WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1')
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11263 test cases.
(48)Address already in use: make_sock: could not bind to address 127.0.0.1:8080
no listening sockets available, shutting down
Unable to open logs
Timed out waiting for httpd to start at WebKitTools/Scripts/run-webkit-tests line 1466, <IN> line 20152.

[Eric Seidel (no email)]

Comment on attachment 39328 [details]
patch with pure js layout test

Sorry, that's clearly not your fault. :)  I happen to be running another service on 8080 on the commit machine when your patch went through the queue.

[WebKit Commit Bot]

Comment on attachment 39328 [details]
patch with pure js layout test

Clearing flags on attachment: 39328

Committed r48436: <http://trac.webkit.org/changeset/48436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.