From 679033d118ac312683906f1159edb5d424324460 Mon Sep 17 00:00:00 2001 From: sushuang Date: Wed, 2 Jan 2019 22:30:01 +0800 Subject: [PATCH] Fix hover style bug. --- src/util/graphic.js | 139 ++++++++++++++++++++++++++++++++++--------- test/hoverStyle.html | 104 +++++++++++++++++++++++++++++++- 2 files changed, 214 insertions(+), 29 deletions(-) diff --git a/src/util/graphic.js b/src/util/graphic.js index f44b8442f..b9abeeba1 100644 --- a/src/util/graphic.js +++ b/src/util/graphic.js @@ -48,6 +48,7 @@ var mathMax = Math.max; var mathMin = Math.min; var EMPTY_OBJ = {}; +var Z2_LIFT_VALUE = 1; /** * Extend shape with parameters @@ -266,11 +267,12 @@ function cacheElementStl(el) { var hoverStyle = el.__hoverStl; if (!hoverStyle) { - el.__normalStl = null; + el.__cachedNormalStl = el.__cachedNormalZ2 = null; return; } - var normalStyle = el.__normalStl = {}; + var normalStyle = el.__cachedNormalStl = {}; + el.__cachedNormalZ2 = el.z2; var elStyle = el.style; for (var name in hoverStyle) { @@ -308,9 +310,6 @@ function doSingleEnterHover(el) { targetStyle = elTarget.style; } - // Consider case: only `position: 'top'` is set on emphasis, then text - // color should be returned to `autoColor`, rather than remain '#fff'. - // So we should rollback then apply again after style merging. rollbackDefaultTextStyle(targetStyle); if (!useHoverLayer) { @@ -345,7 +344,7 @@ function doSingleEnterHover(el) { if (!useHoverLayer) { el.dirty(false); - el.z2 += 1; + el.z2 += Z2_LIFT_VALUE; } } @@ -356,32 +355,36 @@ function setDefaultHoverFillStroke(targetStyle, hoverStyle, prop) { } function doSingleLeaveHover(el) { - if (el.__highlighted) { - doSingleRestoreHoverStyle(el); - el.__highlighted = false; + var highlighted = el.__highlighted; + + if (!highlighted) { + return; } -} -function doSingleRestoreHoverStyle(el) { - var highlighted = el.__highlighted; + el.__highlighted = false; if (highlighted === 'layer') { el.__zr && el.__zr.removeHover(el); } else if (highlighted) { var style = el.style; - var normalStl = el.__normalStl; + var normalStl = el.__cachedNormalStl; if (normalStl) { rollbackDefaultTextStyle(style); - // Consider null/undefined value, should use // `setStyle` but not `extendFrom(stl, true)`. el.setStyle(normalStl); - applyDefaultTextStyle(style); - - el.z2 -= 1; + el.__cachedNormalStl = null; + } + // `__cachedNormalZ2` will not be reset if calling `setElementHoverStyle` + // when `el` is on emphasis state. So here by comparing with 1, we try + // hard to make the bug case rare. + var normalZ2 = el.__cachedNormalZ2; + if (el.z2 - normalZ2 === Z2_LIFT_VALUE) { + el.z2 = normalZ2; + el.__cachedNormalZ2 = null; } } } @@ -395,7 +398,10 @@ function traverseCall(el, method) { } /** - * Set hover style of element. + * Set hover style (namely "emphasis style") of element, based on the current + * style of the given `el`. + * This method should be called after all of the normal styles have been adopted + * to the `el`. See the reason on `setHoverStyle`. * * @param {module:zrender/Element} el Should not be `zrender/container/Group`. * @param {Object|boolean} [hoverStl] The specified hover style. @@ -407,11 +413,29 @@ function traverseCall(el, method) { * @param {boolean} [opt.hoverSilentOnTouch=false] See `graphic.setAsHoverStyleTrigger` */ export function setElementHoverStyle(el, hoverStl) { + // For performance consideration, it might be better to make the "hover style" only the + // difference properties from the "normal style", but not a entire copy of all styles. hoverStl = el.__hoverStl = hoverStl !== false && (hoverStl || {}); el.__hoverStlDirty = true; + // FIXME + // It is not completely right to save "normal"/"emphasis" flag on elements. + // It probably should be saved on `data` of series. Consider the cases: + // (1) A highlighted elements are moved out of the view port and re-enter + // again by dataZoom. + // (2) call `setOption` and replace elements totally when they are highlighted. if (el.__highlighted) { + // Consider the case: + // The styles of a highlighted `el` is being updated. The new "emphasis style" + // should be adapted to the `el`. Notice here new "normal styles" should have + // been set outside and the cached "normal style" is out of date. + el.__cachedNormalStl = null; + // Do not clear `__cachedNormalZ2` here, because setting `z2` is not a constraint + // of this method. In most cases, `z2` is not set and hover style should be able + // to rollback. Of course, that would bring bug, but only in a rare case, see + // `doSingleLeaveHover` for details. doSingleLeaveHover(el); + doSingleEnterHover(el); } } @@ -460,12 +484,29 @@ function leaveEmphasis() { } /** - * Set hover style of element. + * Set hover style (namely "emphasis style") of element, + * based on the current style of the given `el`. + * + * (1) + * **CONSTRAINTS** for this method: + * This method MUST be called after all of the normal styles having been adopted + * to the `el`. + * The input `hoverStyle` (that is, "emphasis style") MUST be the subset of the + * "normal style" having been set to the el. + * `color` MUST be one of the "normal styles" (because color might be lifted as + * a default hover style). * - * [Caveat]: - * This method can be called repeatly and achieve the same result. + * The reason: this method treat the current style of the `el` as the "normal style" + * and cache them when enter/update the "emphasis style". Consider the case: the `el` + * is in "emphasis" state and `setOption`/`dispatchAction` trigger the style updating + * logic, where the el should shift from the original emphasis style to the new + * "emphasis style" and should be able to "downplay" back to the new "normal style". * - * [Usage]: + * Indeed, it is error-prone to make a interface has so many constraints, but I have + * not found a better solution yet to fit the backward compatibility, performance and + * the current programming style. + * + * (2) * Call the method for a "root" element once. Do not call it for each descendants. * If the descendants elemenets of a group has itself hover style different from the * root group, we can simply mount the style on `el.hoverStyle` for them, but should @@ -520,6 +561,7 @@ export function setAsHoverStyleTrigger(el, opt) { } /** + * See more info in `setTextStyleCommon`. * @param {Object|module:zrender/graphic/Style} normalStyle * @param {Object} emphasisStyle * @param {module:echarts/model/Model} normalModel @@ -592,6 +634,7 @@ export function setLabelStyle( /** * Set basic textStyle properties. + * See more info in `setTextStyleCommon`. * @param {Object|module:zrender/graphic/Style} textStyle * @param {module:echarts/model/Model} model * @param {Object} [specifiedTextStyle] Can be overrided by settings in model. @@ -610,6 +653,7 @@ export function setTextStyle( /** * Set text option in the style. + * See more info in `setTextStyleCommon`. * @deprecated * @param {Object} textStyle * @param {module:echarts/model/Model} labelModel @@ -632,7 +676,23 @@ export function setText(textStyle, labelModel, defaultColor) { } /** - * { + * The uniform entry of set text style, that is, retrieve style definitions + * from `model` and set to `textStyle` object. + * + * Never in merge mode, but in overwrite mode, that is, all of the text style + * properties will be set. (Consider the states of normal and emphasis and + * default value can be adopted, merge would make the logic too complicated + * to manage.) + * + * The `textStyle` object can either be a plain object or an instance of + * `zrender/src/graphic/Style`, and either be the style of normal or emphasis. + * After this mothod called, the `textStyle` object can then be used in + * `el.setStyle(textStyle)` or `el.hoverStyle = textStyle`. + * + * Default value will be adopted and `insideRollbackOpt` will be created. + * See `applyDefaultTextStyle` `rollbackDefaultTextStyle` for more details. + * + * opt: { * disableBox: boolean, Whether diable drawing box of block (outer most). * isRectText: boolean, * autoColor: string, specify a color when color is 'auto', @@ -813,14 +873,27 @@ function getAutoColor(color, opt) { return color !== 'auto' ? color : (opt && opt.autoColor) ? opt.autoColor : null; } -// When text position is `inside` and `textFill` not specified, we -// provide a mechanism to auto make text border for better view. But -// text position changing when hovering or being emphasis should be -// considered, where the `insideRollback` enables to restore the style. +/** + * Give some default value to the input `textStyle` object, based on the current settings + * in this `textStyle` object. + * + * The Scenario: + * when text position is `inside` and `textFill` is not specified, we show + * text border by default for better view. But it should be considered that text position + * might be changed when hovering or being emphasis, where the `insideRollback` is used to + * restore the style. + * + * Usage (& NOTICE): + * When a style object (eithor plain object or instance of `zrender/src/graphic/Style`) is + * about to be modified on its text related properties, `rollbackDefaultTextStyle` should + * be called before the modification and `applyDefaultTextStyle` should be called after that. + * (For the case that all of the text related properties is reset, like `setTextStyleCommon` + * does, `rollbackDefaultTextStyle` is not needed to be called). + */ function applyDefaultTextStyle(textStyle) { var opt = textStyle.insideRollbackOpt; - // Only insideRollbackOpt create (setTextStyleCommon used), + // Only `insideRollbackOpt` created (in `setTextStyleCommon`), // applyDefaultTextStyle works. if (!opt || textStyle.textFill != null) { return; @@ -864,6 +937,16 @@ function applyDefaultTextStyle(textStyle) { } } +/** + * Consider the case: in a scatter, + * label: { + * normal: {position: 'inside'}, + * emphasis: {position: 'top'} + * } + * In the normal state, the `textFill` will be set as '#fff' for pretty view (see + * `applyDefaultTextStyle`), but when switching to emphasis state, the `textFill` + * should be retured to 'autoColor', but not keep '#fff'. + */ function rollbackDefaultTextStyle(style) { var insideRollback = style.insideRollback; if (insideRollback) { diff --git a/test/hoverStyle.html b/test/hoverStyle.html index 83b9b788f..a34f6c245 100644 --- a/test/hoverStyle.html +++ b/test/hoverStyle.html @@ -53,6 +53,10 @@ under the License.
+ +
+
+
@@ -61,6 +65,8 @@ under the License.
+ + + + + + + + + + + + +