Bug 51114

Summary: Web Inspector: Up/Down/PageUp/PageDown on a CSS property numeric value commit the value editor
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Severity: Normal CC: abarth, apavlov, bweinstein, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
[PATCH] Suggested fix
[PATCH] Fix reimplemented correctly
eric: review-
[PATCH] Test added pfeldman: review+

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)