131796 2014-04-17 09:36:16 -0700 AX: Malformed tables exposing incorrect col and colSpans 2014-04-19 18:26:47 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 cfleizach cfleizach aboxhall apinheiro commit-queue dmazzoni jcraig jdiggs mario samuel_white webkit-bug-importer oldest_to_newest 1001578 0 cfleizach 2014-04-17 09:36:16 -0700 Check out the table in http://www.blindbargains.com/6thmode.php?m=10831 with VoiceOver Navigating through those elements, the last column is skipped, because colspans are malformed by the author <rdar//problem/16437995> 1001581 1 cfleizach 2014-04-17 09:36:36 -0700 The fix appears to be to use effectiveCols, available on RenderTableCell 1001594 2 229555 cfleizach 2014-04-17 10:01:38 -0700 Created attachment 229555 patch 1001619 3 matthew_hanson 2014-04-17 11:21:53 -0700 <rdar://problem/16437995> 1002274 4 229555 darin 2014-04-19 14:32:33 -0700 Comment on attachment 229555 patch View in context: https://bugs.webkit.org/attachment.cgi?id=229555&action=review > Source/WebCore/accessibility/AccessibilityTableCell.cpp:251 > RenderTableCell* renderCell = toRenderTableCell(m_renderer); I’d suggest using a reference rather than a pointer for this local variable. And I would name it cell. I don’t think a “render” prefix adds anything. > Source/WebCore/accessibility/AccessibilityTableCell.cpp:256 > + RenderTable* table = renderCell->table(); > + if (!table) > + return; Can this ever be null? In practice I don’t think it can, so the code above doesn’t matter. I was going to criticize sometimes returning col/colSpan directly from the element, but I’m not sure it matters. > Source/WebCore/accessibility/AccessibilityTableCell.cpp:258 > + columnRange.first = table->colToEffCol(columnRange.first); I think it’s strange to use columnRange.first here instead of renderCell->col(). Makes the code harder to read for me. 1002314 5 cfleizach 2014-04-19 18:19:24 -0700 (In reply to comment #4) > (From update of attachment 229555 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229555&action=review > > > Source/WebCore/accessibility/AccessibilityTableCell.cpp:251 > > RenderTableCell* renderCell = toRenderTableCell(m_renderer); > > I’d suggest using a reference rather than a pointer for this local variable. And I would name it cell. I don’t think a “render” prefix adds anything. > > > Source/WebCore/accessibility/AccessibilityTableCell.cpp:256 > > + RenderTable* table = renderCell->table(); > > + if (!table) > > + return; > > Can this ever be null? In practice I don’t think it can, so the code above doesn’t matter. I was going to criticize sometimes returning col/colSpan directly from the element, but I’m not sure it matters. > > > Source/WebCore/accessibility/AccessibilityTableCell.cpp:258 > > + columnRange.first = table->colToEffCol(columnRange.first); > > I think it’s strange to use columnRange.first here instead of renderCell->col(). Makes the code harder to read for me. Thanks. I'll make all these changes 1002316 6 cfleizach 2014-04-19 18:26:47 -0700 http://trac.webkit.org/changeset/167558 229555 2014-04-17 10:01:38 -0700 2014-04-19 14:32:33 -0700 patch patch text/plain 6517 cfleizach SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2NzQyOSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE0LTA0LTE3ICBDaHJpcyBG bGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IE1hbGZvcm1lZCB0 YWJsZXMgZXhwb3NpbmcgaW5jb3JyZWN0IGNvbCBhbmQgY29sU3BhbnMKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzMTc5NgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Q6IGFjY2Vzc2liaWxpdHkvdGFi bGUtaW5jb3JyZWN0LWNvbHNwYW4tY2VsbC5odG1sCisKKyAgICAgICAgV2hlbiBhIGRldmVsb3Bl ciBoYXMgc3BlY2lmaWVkIG1hbGZvcm1lZCBjb2xzcGFucywgYWNjZXNzaWJpbGl0eSBpcyByZXBv cnRpbmcgdGhvc2UgdmFsdWVzIGluc3RlYWQgb2YgdGhlIGVmZmVjdGl2ZSBjb2x1bW4gaW5mb3Jt YXRpb24uCisKKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlUYWJsZUNlbGwu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6QWNjZXNzaWJpbGl0eVRhYmxlQ2VsbDo6Y29sdW1uSW5k ZXhSYW5nZSk6CisKIDIwMTQtMDQtMTcgIERhdmlkIEh5YXR0ICA8aHlhdHRAYXBwbGUuY29tPgog CiAgICAgICAgIFtOZXcgTXVsdGljb2x1bW5dIFJFR1JFU1NJT046IENvbHVtbiBzZXQgc3R5bGVz IGRvbid0IHVwZGF0ZSB3aGVuIHpvb21pbmcgZXRjLgpJbmRleDogU291cmNlL1dlYkNvcmUvYWNj ZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5VGFibGVDZWxsLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlUYWJsZUNlbGwuY3BwCShyZXZp c2lvbiAxNjc0MjkpCisrKyBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxp dHlUYWJsZUNlbGwuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNTAsNyArMjUwLDEzIEBACiAgICAg CiAgICAgUmVuZGVyVGFibGVDZWxsKiByZW5kZXJDZWxsID0gdG9SZW5kZXJUYWJsZUNlbGwobV9y ZW5kZXJlcik7CiAgICAgY29sdW1uUmFuZ2UuZmlyc3QgPSByZW5kZXJDZWxsLT5jb2woKTsKLSAg ICBjb2x1bW5SYW5nZS5zZWNvbmQgPSByZW5kZXJDZWxsLT5jb2xTcGFuKCk7ICAgIAorICAgIGNv bHVtblJhbmdlLnNlY29uZCA9IHJlbmRlckNlbGwtPmNvbFNwYW4oKTsKKyAgICBSZW5kZXJUYWJs ZSogdGFibGUgPSByZW5kZXJDZWxsLT50YWJsZSgpOworICAgIGlmICghdGFibGUpCisgICAgICAg IHJldHVybjsKKyAgICAKKyAgICBjb2x1bW5SYW5nZS5maXJzdCA9IHRhYmxlLT5jb2xUb0VmZkNv bChjb2x1bW5SYW5nZS5maXJzdCk7CisgICAgY29sdW1uUmFuZ2Uuc2Vjb25kID0gdGFibGUtPmNv bFRvRWZmQ29sKHJlbmRlckNlbGwtPmNvbCgpICsgcmVuZGVyQ2VsbC0+Y29sU3BhbigpKSAtIGNv bHVtblJhbmdlLmZpcnN0OwogfQogICAgIAogQWNjZXNzaWJpbGl0eU9iamVjdCogQWNjZXNzaWJp bGl0eVRhYmxlQ2VsbDo6dGl0bGVVSUVsZW1lbnQoKSBjb25zdApJbmRleDogTGF5b3V0VGVzdHMv Q2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2aXNpb24g MTY3NDI5KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTMgQEAKKzIwMTQtMDQtMTcgIENocmlzIEZsZWl6YWNoICA8Y2ZsZWl6YWNoQGFwcGxlLmNv bT4KKworICAgICAgICBBWDogTWFsZm9ybWVkIHRhYmxlcyBleHBvc2luZyBpbmNvcnJlY3QgY29s IGFuZCBjb2xTcGFucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTMxNzk2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBhY2Nlc3NpYmlsaXR5L3RhYmxlLWluY29ycmVjdC1jb2xzcGFuLWNlbGwtZXhwZWN0 ZWQudHh0OiBBZGRlZC4KKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L3RhYmxlLWluY29ycmVjdC1j b2xzcGFuLWNlbGwuaHRtbDogQWRkZWQuCisKIDIwMTQtMDQtMTcgIERhdmlkIEh5YXR0ICA8aHlh dHRAYXBwbGUuY29tPgogCiAgICAgICAgIFtOZXcgTXVsdGljb2x1bW5dIFJFR1JFU1NJT046IENv bHVtbiBzZXQgc3R5bGVzIGRvbid0IHVwZGF0ZSB3aGVuIHpvb21pbmcgZXRjLgpJbmRleDogTGF5 b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS90YWJsZS1pbmNvcnJlY3QtY29sc3Bhbi1jZWxsLWV4cGVj dGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9hY2Nlc3NpYmlsaXR5L3RhYmxlLWlu Y29ycmVjdC1jb2xzcGFuLWNlbGwtZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQorKysgTGF5b3V0 VGVzdHMvYWNjZXNzaWJpbGl0eS90YWJsZS1pbmNvcnJlY3QtY29sc3Bhbi1jZWxsLWV4cGVjdGVk LnR4dAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDIyIEBACitIb21lCUhlbHAJTG9naW4KKwlU aXAgdXMgb24gbmV3cworVGhpcyB0ZXN0cyB0aGUgcm93UmFuZ2UgYW5kIGNvbHVtblJhbmdlIGF0 dHJpYnV0ZXMgb2YgdGFibGUgY2VsbHMgaW4gYSB0YWJsZSB3aXRoIGNvbHNwYW5zIHRoYXQgYXJl IGludmFsaWQuCisKK09uIHN1Y2Nlc3MsIHlvdSB3aWxsIHNlZSBhIHNlcmllcyBvZiAiUEFTUyIg bWVzc2FnZXMsIGZvbGxvd2VkIGJ5ICJURVNUIENPTVBMRVRFIi4KKworCitUYWJsZTogUm93czog MiwgQ29sdW1uczogMworW1RhYmxlIGNlbGwgYXQgcm93OiAwLCBjb2x1bW46IDBdIFJvdyByYW5n ZTogezAsIDF9LCBDb2x1bW4gcmFuZ2U6IHswLCAxfQorCVBBU1NFRCBDZWxsIGlzIHNhbWUgY2Vs bCBhdCAoMCwgMCkKK1tUYWJsZSBjZWxsIGF0IHJvdzogMSwgY29sdW1uOiAwXSBSb3cgcmFuZ2U6 IHsxLCAxfSwgQ29sdW1uIHJhbmdlOiB7MCwgMn0KKwlQQVNTRUQgQ2VsbCBpcyBzYW1lIGNlbGwg YXQgKDEsIDApCisJUEFTU0VEIENlbGwgaXMgc2FtZSBjZWxsIGF0ICgxLCAxKQorW1RhYmxlIGNl bGwgYXQgcm93OiAxLCBjb2x1bW46IDFdIFJvdyByYW5nZTogezEsIDF9LCBDb2x1bW4gcmFuZ2U6 IHswLCAyfQorCVBBU1NFRCBDZWxsIGlzIHNhbWUgY2VsbCBhdCAoMSwgMCkKKwlQQVNTRUQgQ2Vs bCBpcyBzYW1lIGNlbGwgYXQgKDEsIDEpCitbVGFibGUgY2VsbCBhdCByb3c6IDEsIGNvbHVtbjog Ml0gUm93IHJhbmdlOiB7MSwgMX0sIENvbHVtbiByYW5nZTogezIsIDF9CisJUEFTU0VEIENlbGwg aXMgc2FtZSBjZWxsIGF0ICgxLCAyKQorUEFTUyBzdWNjZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQor CitURVNUIENPTVBMRVRFCisKSW5kZXg6IExheW91dFRlc3RzL2FjY2Vzc2liaWxpdHkvdGFibGUt aW5jb3JyZWN0LWNvbHNwYW4tY2VsbC5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Fj Y2Vzc2liaWxpdHkvdGFibGUtaW5jb3JyZWN0LWNvbHNwYW4tY2VsbC5odG1sCShyZXZpc2lvbiAw KQorKysgTGF5b3V0VGVzdHMvYWNjZXNzaWJpbGl0eS90YWJsZS1pbmNvcnJlY3QtY29sc3Bhbi1j ZWxsLmh0bWwJKHdvcmtpbmcgY29weSkKQEAgLTAsMCArMSw2NyBAQAorPCFET0NUWVBFIEhUTUwg UFVCTElDICItLy9JRVRGLy9EVEQgSFRNTC8vRU4iPgorPGh0bWw+Cis8aGVhZD4KKzxzY3JpcHQg c3JjPSIuLi9yZXNvdXJjZXMvanMtdGVzdC1wcmUuanMiPjwvc2NyaXB0PgorPC9oZWFkPgorPGJv ZHkgaWQ9ImJvZHkiPgorCis8dGFibGUgaWQ9InRhYmxlIiBib3JkZXI9IjEiIGNlbGxzcGFjaW5n PSIxIj4KKzx0Ym9keT48dHI+PHRkPjx0YWJsZSBib3JkZXI9IjEiIHdpZHRoPSIxMDAlIj48dGJv ZHk+Cis8dHI+PHRkPkhvbWU8L3RkPgorPHRkPkhlbHA8L3RkPgorPHRkPkxvZ2luPC90ZD48L3Ry PjwvdGJvZHk+PC90YWJsZT4KKzwvdGQ+PC90cj48dHI+PHRkIGNvbHNwYW49IjQiPgorPGlucHV0 IHR5cGU9InRleHQiPjwvdGQ+Cis8dGQgY29sc3Bhbj0iMiIgaWQ9InRhYmxlY2VsbCI+VGlwIHVz IG9uIG5ld3M8L3RkPgorPC90cj48L3Rib2R5PjwvdGFibGU+CisKKworPHAgaWQ9ImRlc2NyaXB0 aW9uIj48L3A+Cis8ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKworPHNjcmlwdD4KKworICAgIGRl c2NyaXB0aW9uKCJUaGlzIHRlc3RzIHRoZSByb3dSYW5nZSBhbmQgY29sdW1uUmFuZ2UgYXR0cmli dXRlcyBvZiB0YWJsZSBjZWxscyBpbiBhIHRhYmxlIHdpdGggY29sc3BhbnMgdGhhdCBhcmUgaW52 YWxpZC4iKTsKKworICAgIGZ1bmN0aW9uIHBhcnNlUmFuZ2VTdHJpbmcoc3RyaW5nKSB7CisgICAg ICAgc3RyaW5nID0gc3RyaW5nLnJlcGxhY2UoLyh7fH0pLywgIiIpOworICAgICAgIHZhciBudW1z ID0gc3RyaW5nLnNwbGl0KCIsIik7CisgICAgICAgcmV0dXJuIG5ldyBBcnJheShwYXJzZUludChu dW1zWzBdKSwgcGFyc2VJbnQobnVtc1sxXSkpOworICAgIH0KKworICAgIGlmICh3aW5kb3cuYWNj ZXNzaWJpbGl0eUNvbnRyb2xsZXIpIHsKKyAgICAgICAgdmFyIHRhYmxlID0gYWNjZXNzaWJpbGl0 eUNvbnRyb2xsZXIuYWNjZXNzaWJsZUVsZW1lbnRCeUlkKCJ0YWJsZSIpOworICAgICAgICB2YXIg cm93Q291bnQgPSB0YWJsZS5yb3dDb3VudDsKKyAgICAgICAgdmFyIGNvbHVtbkNvdW50ID0gdGFi bGUuY29sdW1uQ291bnQ7CisgICAgICAgIGRlYnVnKCJUYWJsZTogUm93czogIiArIHJvd0NvdW50 ICsgIiwgQ29sdW1uczogIiArIGNvbHVtbkNvdW50KTsKKyAgICAgICAgZm9yICh2YXIgcm93ID0g MDsgcm93IDwgcm93Q291bnQ7IHJvdysrKSB7CisgICAgICAgICAgICBmb3IgKHZhciBjb2wgPSAw OyBjb2wgPCBjb2x1bW5Db3VudDsgY29sKyspIHsKKyAgICAgICAgICAgICAgICB2YXIgdGFibGVD ZWxsID0gdGFibGUuY2VsbEZvckNvbHVtbkFuZFJvdyhjb2wsIHJvdyk7CisgICAgICAgICAgICAg ICAgaWYgKCF0YWJsZUNlbGwgfHwgIXRhYmxlQ2VsbC5pc1ZhbGlkKQorICAgICAgICAgICAgICAg ICAgICBjb250aW51ZTsKKworICAgICAgICAgICAgICAgIHZhciByb3dJbmRleFJhbmdlID0gdGFi bGVDZWxsLnJvd0luZGV4UmFuZ2UoKTsKKyAgICAgICAgICAgICAgICB2YXIgY29sSW5kZXhSYW5n ZSA9IHRhYmxlQ2VsbC5jb2x1bW5JbmRleFJhbmdlKCk7CisgICAgICAgICAgICAgICAgZGVidWco IltUYWJsZSBjZWxsIGF0IHJvdzogIiArIHJvdyArICIsIGNvbHVtbjogIiArIGNvbCArICJdIFJv dyByYW5nZTogIiArIHJvd0luZGV4UmFuZ2UgKyAiLCBDb2x1bW4gcmFuZ2U6ICIgKyBjb2xJbmRl eFJhbmdlKTsKKyAgICAgICAgICAgICAgICB2YXIgcm93SW5mbyA9IHBhcnNlUmFuZ2VTdHJpbmco cm93SW5kZXhSYW5nZSk7CisgICAgICAgICAgICAgICAgdmFyIGNvbEluZm8gPSBwYXJzZVJhbmdl U3RyaW5nKGNvbEluZGV4UmFuZ2UpOworICAgICAgICAgICAgICAgIGZvciAodmFyIHN1YnJvdyA9 IHJvd0luZm9bMF07IHN1YnJvdyA8IHJvd0luZm9bMF0gKyByb3dJbmZvWzFdOyBzdWJyb3crKykg eworICAgICAgICAgICAgICAgICAgICBmb3IgKHZhciBzdWJjb2wgPSBjb2xJbmZvWzBdOyBzdWJj b2wgPCBjb2xJbmZvWzBdICsgY29sSW5mb1sxXTsgc3ViY29sKyspIHsKKyAgICAgICAgICAgICAg ICAgICAgICAgIHZhciB0ZXN0VGFibGVDZWxsID0gdGFibGUuY2VsbEZvckNvbHVtbkFuZFJvdyhz dWJjb2wsIHN1YnJvdyk7CisgICAgICAgICAgICAgICAgICAgICAgICBpZiAoIXRlc3RUYWJsZUNl bGwgfHwgIXRlc3RUYWJsZUNlbGwuaXNWYWxpZCkKKyAgICAgICAgICAgICAgICAgICAgICAgICAg ICBjb250aW51ZTsKKworICAgICAgICAgICAgICAgICAgICAgICAgdmFyIHNhbWUgPSB0YWJsZUNl bGwuaXNFcXVhbCh0ZXN0VGFibGVDZWxsKTsKKyAgICAgICAgICAgICAgICAgICAgICAgIHZhciBw YXNzZWQgPSBzYW1lID8gIlBBU1NFRCIgOiAiRkFJTEVEIjsKKyAgICAgICAgICAgICAgICAgICAg ICAgIGRlYnVnKCJcdCIgKyBwYXNzZWQgKyAiIENlbGwgaXMgc2FtZSBjZWxsIGF0ICgiICsgc3Vi cm93ICsgIiwgIiArIHN1YmNvbCArICIpIik7CisgICAgICAgICAgICAgICAgICAgIH0KKyAgICAg ICAgICAgICAgICB9CisgICAgICAgICAgICB9CisgICAgICAgIH0KKyAgICB9CisKKzwvc2NyaXB0 PgorCis8c2NyaXB0IHNyYz0iLi4vcmVzb3VyY2VzL2pzLXRlc3QtcG9zdC5qcyI+PC9zY3JpcHQ+ Cis8L2JvZHk+Cis8L2h0bWw+Cg==