Our current heuristics are very conservative, with the intention of avoiding false positives. We can probably relax some of these constraints when we've detected the presence of a main article element on the page. (e.g. NYTimes)
Created attachment 344822 [details] Patch
Comment on attachment 344822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344822&action=review I have not checked this patch for correctness or read through it as I am tired. Just noticed some very minor issues. > Source/WebCore/ChangeLog:8 > + Our current heuristics for triggering the signficant rendered text layout milestone are very conservative, with signficant => significant > Source/WebCore/dom/Document.cpp:7715 > + static unsigned maxNumberOfArticlesBeforeIgnoringMainContentArticle = 10; static => const > Source/WebCore/dom/Document.cpp:7720 > + static float mainContentArticleHeightFactor = 4; Ditto. > Source/WebCore/dom/Document.cpp:7721 > + static float mainContentArticleMinimumHeight = 1000; Ditto.
Created attachment 344827 [details] Patch
Comment on attachment 344822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344822&action=review >> Source/WebCore/ChangeLog:8 >> + Our current heuristics for triggering the signficant rendered text layout milestone are very conservative, with > > signficant => significant Good catch! >> Source/WebCore/dom/Document.cpp:7715 >> + static unsigned maxNumberOfArticlesBeforeIgnoringMainContentArticle = 10; > > static => const Changed to const.
<rdar://problem/42104637>
Comment on attachment 344827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344827&action=review I think we need another iteration since this keeping track of elements in a document is really trickily. I can't r+ this until I see the final patch. > Source/WebCore/dom/Document.cpp:7721 > + const float mainContentArticleMinimumHeight = 1000; This should probably a factor of the viewport size or width. A hard coded height in px doesn't make much sense. > Source/WebCore/dom/Element.cpp:1756 > + if (UNLIKELY(hasTagName(articleTag) && newDocument)) Don't do this in Element::insertedIntoAncestor. Add HTMLArticleElement::insertedIntoAncestor instead. > Source/WebCore/dom/Element.cpp:1806 > + if (UNLIKELY(hasTagName(articleTag))) Ditto.
Comment on attachment 344827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344827&action=review >> Source/WebCore/dom/Document.cpp:7721 >> + const float mainContentArticleMinimumHeight = 1000; > > This should probably a factor of the viewport size or width. > A hard coded height in px doesn't make much sense. Will fix! >> Source/WebCore/dom/Element.cpp:1756 >> + if (UNLIKELY(hasTagName(articleTag) && newDocument)) > > Don't do this in Element::insertedIntoAncestor. Add HTMLArticleElement::insertedIntoAncestor instead. HTMLArticleElement doesn't exist. Should I introduce it?
Comment on attachment 344827 [details] Patch Hm... okay. I guess we'd have to keep it in HTMLElement in that case.
Created attachment 344889 [details] Address feedback.
Created attachment 344909 [details] Move registration logic to HTMLElement.
Comment on attachment 344909 [details] Move registration logic to HTMLElement. Attachment 344909 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8522178 New failing tests: accessibility/roles-exposed.html
Created attachment 344914 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 344909 [details] Move registration logic to HTMLElement. Attachment 344909 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8522219 New failing tests: accessibility/roles-exposed.html
Created attachment 344915 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 344920 [details] Move registration logic to HTMLElement (v2)
(In reply to Ryosuke Niwa from comment #8) > Comment on attachment 344827 [details] > Patch > > Hm... okay. I guess we'd have to keep it in HTMLElement in that case. @Ryosuke — does https://bugs.webkit.org/attachment.cgi?id=344920&action=review look reasonable? If so, I'll send it off to commit queue.
Comment on attachment 344920 [details] Move registration logic to HTMLElement (v2) View in context: https://bugs.webkit.org/attachment.cgi?id=344920&action=review > Source/WebCore/html/HTMLElement.cpp:404 > +Node::InsertedIntoAncestorResult HTMLElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree) I think we should just go with the check in Element. This patch introduces an extra function call.
(In reply to Ryosuke Niwa from comment #17) > Comment on attachment 344920 [details] > Move registration logic to HTMLElement (v2) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344920&action=review > > > Source/WebCore/html/HTMLElement.cpp:404 > > +Node::InsertedIntoAncestorResult HTMLElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree) > > I think we should just go with the check in Element. This patch introduces > an extra function call. 👍 Will do.
Created attachment 344927 [details] Patch for landing
Comment on attachment 344927 [details] Patch for landing Clearing flags on attachment: 344927 Committed r233794: <https://trac.webkit.org/changeset/233794>
All reviewed patches have been landed. Closing bug.