227907 2021-07-13 09:38:57 -0700 Check for dialog existence in top layer in showModal/close 2021-08-17 09:35:46 -0700 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 229149 84635 1 ntim ntim cdumez changseok cmarcelo darin esprehn+autocc ews-watchlist gyuyoung.kim kangil.han koivisto webkit-bug-importer oldest_to_newest 1776648 0 ntim 2021-07-13 09:38:57 -0700 Current behaviour is wrong, especially for showModal, since it will wrongly shuffle around the order, if the dialog is already in the top layer. This needs a isInTopLayer method. 1776650 1 webkit-bug-importer 2021-07-13 09:39:29 -0700 <rdar://problem/80523132> 1784092 2 435479 ntim 2021-08-13 06:23:44 -0700 Created attachment 435479 Patch 1784098 3 ntim 2021-08-13 07:16:42 -0700 Committed r281014 (240504@main): <https://commits.webkit.org/240504@main> 1784102 4 435479 darin 2021-08-13 07:25:08 -0700 Comment on attachment 435479 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435479&action=review > Source/WebCore/html/HTMLDialogElement.cpp:69 > + if (!isInTopLayer()) > + document().addToTopLayer(*this); This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice. > Source/WebCore/html/HTMLDialogElement.cpp:89 > + if (isInTopLayer()) > + document().removeFromTopLayer(*this); I don’t see why this change is needed. This would have no effect even if top layer rendering is implemented. 1784183 5 ntim 2021-08-13 12:04:54 -0700 (In reply to Darin Adler from comment #4) > Comment on attachment 435479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435479&action=review > > > Source/WebCore/html/HTMLDialogElement.cpp:69 > > + if (!isInTopLayer()) > > + document().addToTopLayer(*this); > > This is the double hashing solution, looking in topLayerElements once to see > if the element is there and then doing the same hashing in topLayerElements > again to add the element. If the addToTopLayer performed this check, it > could almost certainly do it without hashing twice. addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again". Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept. > > Source/WebCore/html/HTMLDialogElement.cpp:89 > > + if (isInTopLayer()) > > + document().removeFromTopLayer(*this); > > I don’t see why this change is needed. This would have no effect even if top > layer rendering is implemented. I guess it doesn't matter much right now, this is just following spec words: https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-close If subject is in its Document's top layer, then remove it. It might matter a bit more if we add more things in the `removeFromTopLayer` function (e.g. fullscreen API related, rendering related, etc.). 1784279 6 435479 darin 2021-08-13 16:51:35 -0700 Comment on attachment 435479 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435479&action=review I understand your responses, but I’m still a little disappointed that we end up doing double hashing. >>> Source/WebCore/html/HTMLDialogElement.cpp:69 >>> + document().addToTopLayer(*this); >> >> This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice. > > addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again". > > Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept. Understood. We could still refactor to avoid double hashing, it’s just a bit more complex. We’d need to write a "addToTopLayer if not already present" function, and factor things so that it’s only a single hash table lookup. One way to do that is an argument that changes the behavior of addToTopLayer. >>> Source/WebCore/html/HTMLDialogElement.cpp:89 >>> + document().removeFromTopLayer(*this); >> >> I don’t see why this change is needed. This would have no effect even if top layer rendering is implemented. > > I guess it doesn't matter much right now, this is just following spec words: > > https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-close > If subject is in its Document's top layer, then remove it. > > It might matter a bit more if we add more things in the `removeFromTopLayer` function (e.g. fullscreen API related, rendering related, etc.). Sure, but if we add such things, we’d just need to make sure that removeFromTopLayer does nothing if it’s not a top layer. That’s going to be really convenient since hash table removal functions already tell you if the thing was removed or not. 1784549 7 ntim 2021-08-16 09:33:51 -0700 (In reply to Darin Adler from comment #6) > Comment on attachment 435479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435479&action=review > > I understand your responses, but I’m still a little disappointed that we end > up doing double hashing. > > >>> Source/WebCore/html/HTMLDialogElement.cpp:69 > >>> + document().addToTopLayer(*this); > >> > >> This is the double hashing solution, looking in topLayerElements once to see if the element is there and then doing the same hashing in topLayerElements again to add the element. If the addToTopLayer performed this check, it could almost certainly do it without hashing twice. > > > > addToTopLayer adds to the top layer if not in it, and moves to the last position if it is in it. This is what the top layer spec says: "to add an element to the top layer, remove it first then add it again". > > > > Moving this check in addToTopLayer would change that behaviour (which made the test fail). It's also worth noting that the new fullscreen API should re-use this top layer concept. > > Understood. > > We could still refactor to avoid double hashing, it’s just a bit more > complex. We’d need to write a "addToTopLayer if not already present" > function, and factor things so that it’s only a single hash table lookup. > One way to do that is an argument that changes the behavior of addToTopLayer. That sounds reasonable. I filed bug 229149 as followup. I'm unlikely going to take it before I finish the <dialog> work, but feel free to take it. 1784853 8 darin 2021-08-17 09:35:46 -0700 (In reply to Tim Nguyen (:ntim) from comment #7) > That sounds reasonable. I filed bug 229149 as followup. I'm unlikely going > to take it before I finish the <dialog> work, but feel free to take it. Honestly not sure how important it is, but I might do it. 435479 2021-08-13 06:23:44 -0700 2021-08-13 06:58:40 -0700 Patch bug-227907-20210813152343.patch text/plain 3074 ntim U3VidmVyc2lvbiBSZXZpc2lvbjogMjgwOTIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjUwMTU5NjEzNzExMDU0 MWRlYmQ3MDY5YWEzZmYwNjVhNGFmZjM4NC4uY2RjMGE1MmUyZWU4YTJhYTg2ZDIxOWE2YzdlYjM1 M2RmYTIxYmE4MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDIxLTA4LTEzICBUaW0g Tmd1eWVuICA8bnRpbUBhcHBsZS5jb20+CisKKyAgICAgICAgQ2hlY2sgZm9yIGRpYWxvZyBleGlz dGVuY2UgaW4gdG9wIGxheWVyIGluIEhUTUxEaWFsb2dFbGVtZW50OjpzaG93TW9kYWwgJiBjbG9z ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI3OTA3 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGVzdDog aW1wb3J0ZWQvdzNjL3dlYi1wbGF0Zm9ybS10ZXN0cy9odG1sL3NlbWFudGljcy9pbnRlcmFjdGl2 ZS1lbGVtZW50cy90aGUtZGlhbG9nLWVsZW1lbnQvZGlhbG9nLXNob3dNb2RhbC5odG1sCisKKyAg ICAgICAgVGVzdCBleHBlY3RhdGlvbnMgYXJlIHVuY2hhbmdlZCBiZWNhdXNlIHRoZSB0ZXN0IHVz ZXMgZWxlbWVudEZyb21Qb2ludCwgbWVhbmluZyB0aGF0IGJlaGF2aW91ciBkaWZmZXJlbmNlIGlz bid0IG5vdGljZWFibGUKKyAgICAgICAgdW50aWwgdG9wIGxheWVyIHJlbmRlcmluZyBiaXRzIGFy ZSBpbXBsZW1lbnRlZCAod2hpY2ggd291bGQgY2hhbmdlIGVsZW1lbnRGcm9tUG9pbnQncyByZXN1 bHQgYnkgc2h1ZmZsaW5nIHotb3JkZXIgYmFzZWQgb24gdG9wIGxheWVyIGVsZW1lbnRzKS4KKwor ICAgICAgICAqIGRvbS9FbGVtZW50Lmg6CisgICAgICAgIChXZWJDb3JlOjpFbGVtZW50Ojppc0lu VG9wTGF5ZXIgY29uc3QpOgorICAgICAgICAqIGh0bWwvSFRNTERpYWxvZ0VsZW1lbnQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6SFRNTERpYWxvZ0VsZW1lbnQ6OnNob3dNb2RhbCk6CisgICAgICAg IChXZWJDb3JlOjpIVE1MRGlhbG9nRWxlbWVudDo6Y2xvc2UpOgorCiAyMDIxLTA4LTExICBZb3Vl bm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAogICAgICAgICBNZWRpYSBlbGVtZW50IGlz IG5vdCBhbHdheXMgYXV0b3BsYXlpbmcgd2hlbiBnb2luZyBmcm9tIGJhY2tncm91bmQgdG8gZm9y ZWdyb3VuZCBpZiBpdCBpcyBpbml0aWFsbHkgbm90IGluIHZpZXdwb3J0CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudC5oIGIvU291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnQu aAppbmRleCA5YjQwOWNhOWYzZDk5ODc2NWUyMWVlM2U1NzA5ZWQzNjkzODA0MDYwLi41ODJkYjEy M2I2ZDk3NTc3NGVhNWZjMjQyYTgzNjMxYzg0NzNiZTNlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9kb20vRWxlbWVudC5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmgKQEAg LTUxMSw2ICs1MTEsOCBAQCBwdWJsaWM6CiAgICAgY29uc3QgUmVuZGVyU3R5bGUqIGxhc3RTdHls ZUNoYW5nZUV2ZW50U3R5bGUoUHNldWRvSWQpIGNvbnN0OwogICAgIHZvaWQgc2V0TGFzdFN0eWxl Q2hhbmdlRXZlbnRTdHlsZShQc2V1ZG9JZCwgc3RkOjp1bmlxdWVfcHRyPGNvbnN0IFJlbmRlclN0 eWxlPiYmKTsKIAorICAgIGJvb2wgaXNJblRvcExheWVyKCkgY29uc3QgeyByZXR1cm4gZG9jdW1l bnQoKS50b3BMYXllckVsZW1lbnRzKCkuY29udGFpbnMobWFrZVJlZigqY29uc3RfY2FzdDxFbGVt ZW50Kj4odGhpcykpKTsgfQorCiAjaWYgRU5BQkxFKEZVTExTQ1JFRU5fQVBJKQogICAgIGJvb2wg Y29udGFpbnNGdWxsU2NyZWVuRWxlbWVudCgpIGNvbnN0IHsgcmV0dXJuIGhhc05vZGVGbGFnKE5v ZGVGbGFnOjpDb250YWluc0Z1bGxTY3JlZW5FbGVtZW50KTsgfQogICAgIHZvaWQgc2V0Q29udGFp bnNGdWxsU2NyZWVuRWxlbWVudChib29sKTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0 bWwvSFRNTERpYWxvZ0VsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRGlhbG9n RWxlbWVudC5jcHAKaW5kZXggZjJkZTNjNzk0NzVmM2Q3NjFjNzc0MmM1OGIxZWYyMDIxMDI5YTJl Ny4uZWE4OTA2NDZkNzU2MTQ4NjljODNhZGFhNGU0MGM0NmJmNThkNGRmZiAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRGlhbG9nRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MRGlhbG9nRWxlbWVudC5jcHAKQEAgLTY1LDggKzY1LDggQEAgRXhjZXB0 aW9uT3I8dm9pZD4gSFRNTERpYWxvZ0VsZW1lbnQ6OnNob3dNb2RhbCgpCiAKICAgICBtX2lzTW9k YWwgPSB0cnVlOwogCi0gICAgLy8gRklYTUU6IE9ubHkgYWRkIGRpYWxvZyB0byB0b3AgbGF5ZXIg aWYgaXQncyBub3QgYWxyZWFkeSBpbiBpdC4gKHdlYmtpdC5vcmcvYi8yMjc5MDcpCi0gICAgZG9j dW1lbnQoKS5hZGRUb1RvcExheWVyKCp0aGlzKTsKKyAgICBpZiAoIWlzSW5Ub3BMYXllcigpKQor ICAgICAgICBkb2N1bWVudCgpLmFkZFRvVG9wTGF5ZXIoKnRoaXMpOwogCiAgICAgLy8gRklYTUU6 IEFkZCBzdGVwcyA4ICYgOSBmcm9tIHNwZWMuICh3ZWJraXQub3JnL2IvMjI3NTM3KQogCkBAIC04 NSw4ICs4NSw4IEBAIHZvaWQgSFRNTERpYWxvZ0VsZW1lbnQ6OmNsb3NlKGNvbnN0IFN0cmluZyYg cmVzdWx0KQogICAgIGlmICghcmVzdWx0LmlzTnVsbCgpKQogICAgICAgICBtX3JldHVyblZhbHVl ID0gcmVzdWx0OwogCi0gICAgLy8gRklYTUU6IE9ubHkgcmVtb3ZlIGRpYWxvZyBmcm9tIHRvcCBs YXllciBpZiBpdCdzIGluc2lkZSBpdC4gKHdlYmtpdC5vcmcvYi8yMjc5MDcpCi0gICAgZG9jdW1l bnQoKS5yZW1vdmVGcm9tVG9wTGF5ZXIoKnRoaXMpOworICAgIGlmIChpc0luVG9wTGF5ZXIoKSkK KyAgICAgICAgZG9jdW1lbnQoKS5yZW1vdmVGcm9tVG9wTGF5ZXIoKnRoaXMpOwogCiAgICAgLy8g RklYTUU6IEFkZCBzdGVwIDYgZnJvbSBzcGVjLiAod2Via2l0Lm9yZy9iLzIyNzUzNykKIAo=