LibWeb: Fix storage set broadcast event never broadcasting old value

We had skipped some steps in the spec and were:
 * Always broadcasting an old value of null, instead of what it
   actually was previously.
 * Still broadcasting a storage event even if the value had
   not changed in storage compared to the last value.

Fix both issues by returning what the old value is in the setter and
implementing the missing logic.

Libraries/LibWeb/HTML/Storage.cpp

@@ -103,18 +103,34 @@ Optional<String> Storage::get_item(String const& key) const
 WebIDL::ExceptionOr<void> Storage::set_item(String const& key, String const& value)
 {
     // 1. Let oldValue be null.
-    Optional<String> old_value;
-
     // 2. Let reorder be true.
-    // 3. If this's map[key] exists:
+    bool reorder = true;
 
+    // 3. If this's map[key] exists:
+    //     1. Set oldValue to this's map[key].
+    //     2. If oldValue is value, then return.
+    //     3. Set reorder to false.
     // 4. If value cannot be stored, then throw a "QuotaExceededError" DOMException.
     // 5. Set this's map[key] to value.
-    auto error = m_storage_bottle->set(key, value);
-    if (error == WebView::StorageOperationError::QuotaExceededError) {
+
+    auto result = m_storage_bottle->set(key, value);
+
+    if (result.has<WebView::StorageOperationError>())
         return WebIDL::QuotaExceededError::create(realm(), Utf16String::formatted("Unable to store more than {} bytes in storage", *m_storage_bottle->quota()));
+
+    auto old_value = result.get<Optional<String>>();
+
+    if (old_value.has_value()) {
+        if (old_value.value() == value)
+            return {};
+
+        reorder = false;
     }
 
+    // 6. If reorder is true, then reorder this.
+    if (reorder)
+        this->reorder();
+
     // 7. Broadcast this with key, oldValue, and value.
     broadcast(key, old_value, value);
 

Libraries/LibWeb/Page/Page.h

@@ -45,7 +45,7 @@
 #include <LibWeb/PixelUnits.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWeb/UIEvents/KeyCode.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace Web {
 
@@ -385,7 +385,7 @@ public:
     virtual void page_did_update_cookie(Web::Cookie::Cookie const&) { }
     virtual void page_did_expire_cookies_with_time_offset(AK::Duration) { }
     virtual Optional<String> page_did_request_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key) { return {}; }
-    virtual WebView::StorageOperationError page_did_set_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key, [[maybe_unused]] String const& value) { return WebView::StorageOperationError::None; }
+    virtual WebView::StorageSetResult page_did_set_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key, [[maybe_unused]] String const& value) { return WebView::StorageOperationError::QuotaExceededError; }
     virtual void page_did_remove_storage_item([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key, [[maybe_unused]] String const& bottle_key) { }
     virtual Vector<String> page_did_request_storage_keys([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key) { return {}; }
     virtual void page_did_clear_storage([[maybe_unused]] Web::StorageAPI::StorageEndpointType storage_endpoint, [[maybe_unused]] String const& storage_key) { }

Libraries/LibWeb/StorageAPI/StorageBottle.cpp

@@ -119,7 +119,7 @@ Optional<String> LocalStorageBottle::get(String const& key) const
     return m_page->client().page_did_request_storage_item(Web::StorageAPI::StorageEndpointType::LocalStorage, m_storage_key.to_string(), key);
 }
 
-WebView::StorageOperationError LocalStorageBottle::set(String const& key, String const& value)
+WebView::StorageSetResult LocalStorageBottle::set(String const& key, String const& value)
 {
     return m_page->client().page_did_set_storage_item(Web::StorageAPI::StorageEndpointType::LocalStorage, m_storage_key.to_string(), key, value);
 }
@@ -151,8 +151,10 @@ Optional<String> SessionStorageBottle::get(String const& key) const
     return OptionalNone {};
 }
 
-WebView::StorageOperationError SessionStorageBottle::set(String const& key, String const& value)
+WebView::StorageSetResult SessionStorageBottle::set(String const& key, String const& value)
 {
+    auto old_value = get(key);
+
     if (m_quota.has_value()) {
         size_t current_size = 0;
         for (auto const& [existing_key, existing_value] : m_map) {
@@ -167,7 +169,7 @@ WebView::StorageOperationError SessionStorageBottle::set(String const& key, Stri
     }
 
     m_map.set(key, value);
-    return WebView::StorageOperationError::None;
+    return old_value;
 }
 
 void SessionStorageBottle::clear()

Libraries/LibWeb/StorageAPI/StorageBottle.h

@@ -15,7 +15,7 @@
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWeb/StorageAPI/StorageKey.h>
 #include <LibWeb/StorageAPI/StorageType.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace Web::StorageAPI {
 
@@ -34,7 +34,7 @@ public:
     virtual size_t size() const = 0;
     virtual Vector<String> keys() const = 0;
     virtual Optional<String> get(String const&) const = 0;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) = 0;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) = 0;
     virtual void clear() = 0;
     virtual void remove(String const&) = 0;
 
@@ -62,7 +62,7 @@ public:
     virtual size_t size() const override;
     virtual Vector<String> keys() const override;
     virtual Optional<String> get(String const&) const override;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) override;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) override;
     virtual void clear() override;
     virtual void remove(String const&) override;
 
@@ -93,7 +93,7 @@ public:
     virtual size_t size() const override;
     virtual Vector<String> keys() const override;
     virtual Optional<String> get(String const&) const override;
-    virtual WebView::StorageOperationError set(String const& key, String const& value) override;
+    virtual WebView::StorageSetResult set(String const& key, String const& value) override;
     virtual void clear() override;
     virtual void remove(String const&) override;
 

Libraries/LibWebView/StorageJar.cpp

@@ -107,7 +107,7 @@ Optional<String> StorageJar::get_item(StorageEndpointType storage_endpoint, Stri
     return m_transient_storage.get_item(storage_location);
 }
 
-StorageOperationError StorageJar::set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value)
+StorageSetResult StorageJar::set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value)
 {
     StorageLocation storage_location { storage_endpoint, storage_key, bottle_key };
 
@@ -166,8 +166,10 @@ Optional<String> StorageJar::TransientStorage::get_item(StorageLocation const& k
     return {};
 }
 
-StorageOperationError StorageJar::TransientStorage::set_item(StorageLocation const& key, String const& value)
+StorageSetResult StorageJar::TransientStorage::set_item(StorageLocation const& key, String const& value)
 {
+    auto old_value = get_item(key);
+
     u64 current_size = 0;
 
     for (auto const& [existing_key, existing_entry] : m_storage_items) {
@@ -182,7 +184,7 @@ StorageOperationError StorageJar::TransientStorage::set_item(StorageLocation con
         return StorageOperationError::QuotaExceededError;
 
     m_storage_items.set(key, { value, UnixDateTime::now() });
-    return StorageOperationError::None;
+    return old_value;
 }
 
 void StorageJar::TransientStorage::delete_item(StorageLocation const& key)
@@ -262,8 +264,10 @@ Optional<String> StorageJar::PersistedStorage::get_item(StorageLocation const& k
     return result;
 }
 
-StorageOperationError StorageJar::PersistedStorage::set_item(StorageLocation const& key, String const& value)
+StorageSetResult StorageJar::PersistedStorage::set_item(StorageLocation const& key, String const& value)
 {
+    auto old_value = get_item(key);
+
     size_t current_size = 0;
     database.execute_statement(
         statements.calculate_size_excluding_key,
@@ -287,7 +291,7 @@ StorageOperationError StorageJar::PersistedStorage::set_item(StorageLocation con
         value,
         UnixDateTime::now());
 
-    return StorageOperationError::None;
+    return old_value;
 }
 
 void StorageJar::PersistedStorage::delete_item(StorageLocation const& key)

Libraries/LibWebView/StorageJar.h

@@ -14,7 +14,7 @@
 #include <LibRequests/CacheSizes.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWebView/Forward.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 
 namespace WebView {
 
@@ -39,7 +39,7 @@ public:
     ~StorageJar();
 
     Optional<String> get_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key);
-    StorageOperationError set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value);
+    StorageSetResult set_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& bottle_value);
     void remove_item(StorageEndpointType storage_endpoint, String const& storage_key, String const& key);
     void remove_items_accessed_since(UnixDateTime);
     void clear_storage_key(StorageEndpointType storage_endpoint, String const& storage_key);
@@ -62,7 +62,7 @@ private:
     class TransientStorage {
     public:
         Optional<String> get_item(StorageLocation const& key);
-        StorageOperationError set_item(StorageLocation const& key, String const& value);
+        StorageSetResult set_item(StorageLocation const& key, String const& value);
         void delete_item(StorageLocation const& key);
         void delete_items_accessed_since(UnixDateTime);
         void clear(StorageEndpointType storage_endpoint, String const& storage_key);
@@ -80,7 +80,7 @@ private:
 
     struct PersistedStorage {
         Optional<String> get_item(StorageLocation const& key);
-        StorageOperationError set_item(StorageLocation const& key, String const& value);
+        StorageSetResult set_item(StorageLocation const& key, String const& value);
         void delete_item(StorageLocation const& key);
         void delete_items_accessed_since(UnixDateTime);
         void clear(StorageEndpointType storage_endpoint, String const& storage_key);

Libraries/LibWebView/StorageOperationError.h

@@ -1,16 +0,0 @@
-/*
- * Copyright (c) 2025, Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#pragma once
-
-namespace WebView {
-
-enum class StorageOperationError : u8 {
-    None,
-    QuotaExceededError,
-};
-
-}

Libraries/LibWebView/StorageSetResult.h

@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2025, Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
+ * Copyright (c) 2025, Shannon Booth <shannon@serenityos.org>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#pragma once
+
+#include <AK/Optional.h>
+#include <AK/String.h>
+#include <AK/Types.h>
+
+namespace WebView {
+
+enum class StorageOperationError : u8 {
+    QuotaExceededError,
+};
+
+// Error setting the storage item, or the old value if the operation was successful.
+using StorageSetResult = Variant<StorageOperationError, Optional<String>>;
+
+}

Services/WebContent/PageClient.cpp

@@ -542,14 +542,14 @@ Optional<String> PageClient::page_did_request_storage_item(Web::StorageAPI::Stor
     return response->take_value();
 }
 
-WebView::StorageOperationError PageClient::page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value)
+WebView::StorageSetResult PageClient::page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value)
 {
     auto response = client().send_sync_but_allow_failure<Messages::WebContentClient::DidSetStorageItem>(storage_endpoint, storage_key, bottle_key, value);
     if (!response) {
         dbgln("WebContent client disconnected during DidSetStorageItem. Exiting peacefully.");
         exit(0);
     }
-    return response->error();
+    return response->result();
 }
 
 void PageClient::page_did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key)

Services/WebContent/PageClient.h

@@ -17,7 +17,7 @@
 #include <LibWeb/PixelUnits.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
 #include <LibWebView/Forward.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 #include <WebContent/Forward.h>
 
 namespace WebContent {
@@ -160,7 +160,7 @@ private:
     virtual void page_did_update_cookie(Web::Cookie::Cookie const&) override;
     virtual void page_did_expire_cookies_with_time_offset(AK::Duration) override;
     virtual Optional<String> page_did_request_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key) override;
-    virtual WebView::StorageOperationError page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value) override;
+    virtual WebView::StorageSetResult page_did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key, String const& value) override;
     virtual void page_did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key, String const& bottle_key) override;
     virtual Vector<String> page_did_request_storage_keys(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key) override;
     virtual void page_did_clear_storage(Web::StorageAPI::StorageEndpointType storage_endpoint, String const& storage_key) override;

Services/WebContent/WebContentClient.ipc

@@ -24,7 +24,7 @@
 #include <LibWebView/ConsoleOutput.h>
 #include <LibWebView/DOMNodeProperties.h>
 #include <LibWeb/StorageAPI/StorageEndpoint.h>
-#include <LibWebView/StorageOperationError.h>
+#include <LibWebView/StorageSetResult.h>
 #include <LibWebView/Mutation.h>
 #include <LibWebView/PageInfo.h>
 #include <LibWebView/ProcessHandle.h>
@@ -83,7 +83,7 @@ endpoint WebContentClient
     did_update_cookie(Web::Cookie::Cookie cookie) =|
     did_expire_cookies_with_time_offset(AK::Duration offset) =|
     did_request_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key) => (Optional<String> value)
-    did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key, String value) => (WebView::StorageOperationError error)
+    did_set_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key, String value) => (WebView::StorageSetResult result)
     did_remove_storage_item(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key, String bottle_key) => ()
     did_request_storage_keys(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key) => (Vector<String> keys)
     did_clear_storage(Web::StorageAPI::StorageEndpointType storage_endpoint, String storage_key) => ()

Tests/LibWeb/Text/expected/wpt-import/webstorage/event_session_oldvalue.txt

@@ -0,0 +1,6 @@
+Harness status: OK
+
+Found 1 tests
+
+1 Pass
+Pass	oldvalue property test of session event - Session event is fired due to an invocation of the setItem(), clear() methods.

Tests/LibWeb/Text/input/wpt-import/webstorage/event_session_oldvalue.html

@@ -0,0 +1,38 @@
+<!DOCTYPE HTML>
+<html>
+ <head>
+  <title>WebStorage Test: sessionStorage event - oldValue</title>
+  <script src="../resources/testharness.js"></script>
+  <script src="../resources/testharnessreport.js"></script>
+ </head>
+ <body>
+    <h1>event_session_oldValue</h1>
+    <div id="log"></div>
+    <script>
+        async_test(function(t) {
+            sessionStorage.clear();
+            t.add_cleanup(function() { sessionStorage.clear() });
+
+            self.fail = t.step_func(function(msg) {
+                assert_unreached(msg);
+                t.done();
+            });
+
+            var expected = [null, 'user1', null]
+            function onStorageEvent(event) {
+                assert_equals(event.oldValue, expected.shift());
+                if (!expected.length) {
+                    t.done();
+                }
+            }
+
+            window.addEventListener('storage', t.step_func(onStorageEvent), false);
+
+            var el = document.createElement("iframe");
+            el.setAttribute('id', 'ifrm');
+            el.setAttribute('src', 'resources/session_change_item_iframe.html');
+            document.body.appendChild(el);
+        }, "oldvalue property test of session event - Session event is fired due to an invocation of the setItem(), clear() methods.");
+    </script>
+ </body>
+</html>

Tests/LibWeb/Text/input/wpt-import/webstorage/resources/session_change_item_iframe.html

@@ -0,0 +1,18 @@
+<!DOCTYPE HTML>
+<html>
+ <body>
+    <script>
+        if (('sessionStorage' in window) && window.sessionStorage !== null){
+            try {
+                sessionStorage.setItem("name", "user1");
+                sessionStorage.setItem("name", "user2");
+            } catch (e) {
+                parent.fail("setItem method is failed.");
+            }
+            sessionStorage.clear();
+        } else {
+            parent.fail("sessionStorage is not supported.");
+        }
+    </script>
+ </body>
+</html>