Bug 51114 - Web Inspector: Up/Down/PageUp/PageDown on a CSS property numeric value commit the value editor
Summary: Web Inspector: Up/Down/PageUp/PageDown on a CSS property numeric value commit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-15 09:46 PST by Alexander Pavlov (apavlov)
Modified: 2010-12-16 15:38 PST (History)
13 users (show)

See Also:


Attachments
[PATCH] Suggested fix (2.92 KB, patch)
2010-12-15 10:06 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fix reimplemented correctly (1.99 KB, patch)
2010-12-15 10:32 PST, Alexander Pavlov (apavlov)
eric: review-
Details | Formatted Diff | Diff
[PATCH] Test added (5.71 KB, patch)
2010-12-16 05:00 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-12-15 09:46:07 PST
[Page]Up/Down should not commit a CSS property value altogether but instead apply it to the style and continue editing.

Patch to follow
Comment 1 Alexander Pavlov (apavlov) 2010-12-15 10:06:35 PST
Created attachment 76660 [details]
[PATCH] Suggested fix
Comment 2 Yury Semikhatsky 2010-12-15 10:11:55 PST
Comment on attachment 76660 [details]
[PATCH] Suggested fix

View in context: https://bugs.webkit.org/attachment.cgi?id=76660&action=review

> WebCore/inspector/front-end/ElementsPanel.js:-89
> -    this.sidebarPanes.styles.addEventListener("style property toggled", this._stylesPaneEdited, this);

_stylesPaneEdited is now only called from _styleSheetChanged, please get rid of _stylesPaneEdited
Comment 3 Alexander Pavlov (apavlov) 2010-12-15 10:32:41 PST
Created attachment 76665 [details]
[PATCH] Fix reimplemented correctly

pfeldman@ has explained the idea behind the event handling. This patch reverts the erroneous fix from r74038 and augments the stylesheet onRevert handling
Comment 4 Eric Seidel (no email) 2010-12-15 15:25:54 PST
Comment on attachment 76665 [details]
[PATCH] Fix reimplemented correctly

How do we test this?
Comment 5 Alexander Pavlov (apavlov) 2010-12-16 05:00:19 PST
Created attachment 76751 [details]
[PATCH] Test added
Comment 6 Pavel Feldman 2010-12-16 05:12:32 PST
(In reply to comment #4)
> (From update of attachment 76665 [details])
> How do we test this?

Eric, could you please not blindly r- inspector changes that are missing tests without meaningful review comments? They go away from my to review report and this slows down the development.

If you are willing to help us with the testing, as you know, inspector tests became flaky at some point and something in the harness needs to be fixed. I already had similar discussion with Adam the other day.
Comment 7 Alexander Pavlov (apavlov) 2010-12-16 06:10:25 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        M       LayoutTests/inspector/styles-add-blank-property-expected.txt
        M       LayoutTests/inspector/styles-add-blank-property.html
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/ElementsPanel.js
Committed r74185
Comment 8 WebKit Review Bot 2010-12-16 15:38:27 PST
http://trac.webkit.org/changeset/74185 might have broken Leopard Intel Debug (Tests)