Description: Consider a testcase which contains a set of sub-tests all checking if the element has correct data-offset-y(offsetTop). When one of the sub-tests fails, the failure msg is appended to the elements container(in our case, its the sub-test). This extra content changes the offsetTop of other subsequent sub-tests and all sub-tests following the failing test case also fail. This becomes confusing/time consuming when either creating a test-case or debugging test failure.
Created attachment 180166 [details] TestCase
Maybe we should save error messages + their containers into an array and only write them out at the end of the test? If we wanted to be paranoid, we could do it in a try/finally to ensure that we write out the errors even if the test throws an exception, although, it really shouldn't since all the relevant code is in check-layout.js.
(In reply to comment #2) > Maybe we should save error messages + their containers into an array and only write them out at the end of the test? If we wanted to be paranoid, we could do it in a try/finally to ensure that we write out the errors even if the test throws an exception, although, it really shouldn't since all the relevant code is in check-layout.js. Some other options: We currently append PASS/FAIL next to the parent node, or if the parent node has the container class, we append next to the parent's parent. That's kind of hacky. We could add an optional param to checkLayout() for the node to append results to (and make the PASS text more clear like listing the id or class of the passing node). Alternately, we could add an attribute like data-log-results="some-div-id". If we see that, we write the results to some-div-id. Anyway, I think any of these approaches are fine. It's a little weird that for some of the existing writing mode tests the word "PASS" ends up being written vertically.
(In reply to comment #3) > We currently append PASS/FAIL next to the parent node, or if the parent node has the container class, we append next to the parent's parent. That's kind of hacky. We could add an optional param to checkLayout() for the node to append results to (and make the PASS text more clear like listing the id or class of the passing node). I agree this is less hacky, but it's also harder to use with tests that have lots of test-cases. It's hard to match up the failure to the test case. > Alternately, we could add an attribute like data-log-results="some-div-id". If we see that, we write the results to some-div-id. Same issue, right? Also, I don't like adding more work for people to do when writing these tests. > Anyway, I think any of these approaches are fine. It's a little weird that for some of the existing writing mode tests the word "PASS" ends up being written vertically. I agree that it's unfortunate that the logging output affects the tests and is affected by the tests. We could use shadow-dom with resetStyleInheritance. :) More seriously, with my original proposal, we could put them in a separate container div at the end and absolutely position the elements after the test-case to minimize the affects between tests and their output? I'd really like to keep the behavior of having the failure output be next to the test case if possible.
All my suggestions were for an optional parameter. The default behavior would be to do what we currently do. I don't think there are that many tests that need to set a custom output location. We shouldn't need to edit or rebaseline any existing tests. I think it would be hard to know where to place the result using absolute positioning. The other option would be to do nothing. In the attached test case, you can easily avoid the problem by doing checkLayout('body'), although you only get a single PASS or FAIL. If you want a separate PASS/FAIL for each test case, you could wrap each test in a position:relative div. We do that for a lot of the flexbox tests.
Created attachment 180234 [details] Proped Patch
Comment on attachment 180234 [details] Proped Patch Reversing the order in which nodes are processed should ideally fix the issue. @Ojan, @Tony please let me know ur opinions. Also am not sure where to add a test case for this fix... Suggestion req on the same.
Comment on attachment 180234 [details] Proped Patch Attachment 180234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15401804 New failing tests: css3/flexbox/flex-algorithm-with-margins.html css3/flexbox/box-sizing.html css3/flexbox/flex-flow-margins.html css3/flexbox/flex-align.html animations/suspend-resume-animation-events.html css3/flexbox/flex-align-percent-height.html css3/flexbox/flex-align-vertical-writing-mode.html css3/flexbox/align-absolute-child.html css3/flexbox/flex-item-min-size.html css3/flexbox/flex-no-flex.html css3/flexbox/flex-algorithm.html css3/flexbox/columns-height-set-via-top-bottom.html css3/flexbox/flex-flow.html css3/flexbox/flex-align-column.html css3/flexbox/flex-algorithm-min-max.html css3/flexbox/flex-item-child-overflow.html css3/flexbox/flex-flow-margins-auto-size.html css3/flexbox/flex-align-max.html css3/flexbox/auto-height-dynamic.html css3/flexbox/flex-flow-auto-margins.html css3/flexbox/flex-flow-padding.html css3/flexbox/columns-auto-size.html css3/flexbox/flex-align-end.html css3/flexbox/flex-flow-orientations.html css3/flexbox/flex-flow-overflow.html css3/flexbox/flex-align-stretch.html css3/flexbox/flex-flow-auto-margins-no-available-space.html css3/flexbox/flex-flow-border.html css3/flexbox/box-sizing-min-max-sizes.html css3/flexbox/flex-justify-content.html
Comment on attachment 180234 [details] Proped Patch Attachment 180234 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15403871 New failing tests: css3/flexbox/flex-algorithm-with-margins.html css3/flexbox/box-sizing.html css3/flexbox/flex-flow-margins.html css3/flexbox/flex-align.html css3/flexbox/flex-align-percent-height.html css3/flexbox/flex-align-vertical-writing-mode.html css3/flexbox/align-absolute-child.html css3/flexbox/flex-item-min-size.html css3/flexbox/flex-no-flex.html css3/flexbox/flex-algorithm.html css3/flexbox/columns-height-set-via-top-bottom.html css3/flexbox/flex-flow.html css3/flexbox/flex-align-column.html css3/flexbox/flex-algorithm-min-max.html css3/flexbox/flex-item-child-overflow.html css3/flexbox/flex-flow-margins-auto-size.html css3/flexbox/flex-align-max.html css3/flexbox/auto-height-dynamic.html css3/flexbox/flex-flow-auto-margins.html css3/flexbox/flex-flow-padding.html css3/flexbox/columns-auto-size.html css3/flexbox/flex-align-end.html css3/flexbox/flex-flow-orientations.html css3/flexbox/flex-flow-overflow.html compositing/overflow/overflow-auto-with-touch-toggle.html css3/flexbox/flex-align-stretch.html css3/flexbox/flex-flow-auto-margins-no-available-space.html css3/flexbox/flex-flow-border.html css3/flexbox/box-sizing-min-max-sizes.html css3/flexbox/flex-justify-content.html
Comment on attachment 180234 [details] Proped Patch CONSOLE MESSAGE: line 163: Uncaught TypeError: [object Array] is not a function is what the tests are spewing.
Created attachment 180392 [details] Patch
(In reply to comment #10) > (From update of attachment 180234 [details]) > CONSOLE MESSAGE: line 163: Uncaught TypeError: [object Array] is not a function is what the tests are spewing. > I extremely sorry for this. Have uploaded another patch, with which test case do not seem to fail.
Comment on attachment 180392 [details] Patch Seems fine to me. Ojan might have an opinion, but he won't be back from vacation until January.
Comment on attachment 180392 [details] Patch I'm a bit torn on whether this is the best solution. I still somewhat prefer the approach of saving everything in an error array and then writing out all the errors at the end. But, I don't see any immediate problems with this simpler approach.
Comment on attachment 180392 [details] Patch Clearing flags on attachment: 180392 Committed r141147: <http://trac.webkit.org/changeset/141147>
All reviewed patches have been landed. Closing bug.